PRMT-4568 - ehr-repository - Remove PostgreSQL Implementation and Sequelize Dependencies by joefong-nhs · Pull Request #73 · NHSDigital/orphaned-record-continuity

joefong-nhs · 2024-03-11T13:32:39Z

No description provided.

…tring to address a mac specific problem

…for local testing

…odb integration test

… separating model and repository layer

…racker table

…termine latest record

…js to new dynamodb-based implementation

…. Store current progress

…rrent progress

…ark and ticket number to old postgres related codes

…erraform things)

MohammadIqbalAD-NHS · 2024-03-12T10:20:22Z

package.json

@@ -18,10 +18,7 @@
    "build": "babel src -d build --ignore '**/*.test.js' --ignore '**/__mocks__/*' && cp src/*.json build",
    "start": "node build/server.js",
    "start:local": "npm run db:migrate ; babel-node -r dotenv/config src/server.js",


Doesn't this line npm run db:migrate need to be removed?

Sure! Just removed this

MohammadIqbalAD-NHS · 2024-03-12T11:04:44Z

terraform-db-roles/dynamodb.tf

+data "aws_iam_policy_document" "ehr-transfer-tracker-db-access" {
+  statement {
+    actions = [
+      "dynamodb:GetItem",
+      "dynamodb:PutItem",
+      "dynamodb:UpdateItem",
+      "dynamodb:Query"
+    ]
+    resources = [
+      "arn:aws:dynamodb:${var.region}:${data.aws_caller_identity.current.account_id}:table/${var.environment}-ehr-transfer-tracker"
+    ]
+  }
+}
+
+resource "aws_iam_policy" "ehr-transfer-tracker-db-access" {
+  name   = "${var.environment}-${var.component_name}-transfer-tracker-db-access"
+  policy = data.aws_iam_policy_document.ehr-transfer-tracker-db-access.json
+}
+
+# Grant ECS Task permissions to access the transfer tracker db
+resource "aws_iam_role_policy_attachment" "dynamodb_application_user_policy_attach" {
+  role       = "${var.environment}-${var.component_name}-EcsTaskRole"
+  policy_arn = aws_iam_policy.ehr-transfer-tracker-db-access.arn
+}


I notice sometimes you've named the resource identifiers with hyphens and sometimes with underscores. Although there's no strict guideline on how to name identifiers, generally underscores are preferred, which is how we've been naming new Terraform code we've been writing.

Suggested change

data "aws_iam_policy_document" "ehr-transfer-tracker-db-access" {

statement {

actions = [

"dynamodb:GetItem",

"dynamodb:PutItem",

"dynamodb:UpdateItem",

"dynamodb:Query"

]

resources = [

"arn:aws:dynamodb:${var.region}:${data.aws_caller_identity.current.account_id}:table/${var.environment}-ehr-transfer-tracker"

]

}

}

resource "aws_iam_policy" "ehr-transfer-tracker-db-access" {

name = "${var.environment}-${var.component_name}-transfer-tracker-db-access"

policy = data.aws_iam_policy_document.ehr-transfer-tracker-db-access.json

}

# Grant ECS Task permissions to access the transfer tracker db

resource "aws_iam_role_policy_attachment" "dynamodb_application_user_policy_attach" {

role = "${var.environment}-${var.component_name}-EcsTaskRole"

policy_arn = aws_iam_policy.ehr-transfer-tracker-db-access.arn

}

data "aws_iam_policy_document" "ehr_transfer_tracker_db_access" {

statement {

actions = [

"dynamodb:GetItem",

"dynamodb:PutItem",

"dynamodb:UpdateItem",

"dynamodb:Query"

]

resources = [

"arn:aws:dynamodb:${var.region}:${data.aws_caller_identity.current.account_id}:table/${var.environment}-ehr-transfer-tracker"

]

}

}

resource "aws_iam_policy" "ehr_transfer_tracker_db_access" {

name = "${var.environment}-${var.component_name}-transfer-tracker-db-access"

policy = data.aws_iam_policy_document.ehr-transfer-tracker-db-access.json

}

Thanks for spotting this! Just changed the hyphens to underscore

MohammadIqbalAD-NHS · 2024-03-12T11:46:55Z

src/services/database/dynamodb-client.js

+
+export const getDynamodbClient = () => {
+  const clientConfig = {
+    region: process.env.AWS_DEFAULT_REGION ?? 'eu-west-2',


Is this comma at the end needed?

MohammadIqbalAD-NHS · 2024-03-13T13:37:21Z

src/api/patients/patient-details-controller.js

+      // TODO: remove this duplicated `conversationIdFromEhrIn` field,
+      // after updating ehr-out to use the field name "inboundConversationId"


Could you please put the Ticket number on this TODO, so we know what it refers to?

Sure! Just added ticket number to TODO

MohammadIqbalAD-NHS · 2024-03-13T13:40:56Z

src/errors/errors.js

+
+export const errorMessages = {
+  HealthRecordNotFound: 'No complete health record was found with given criteria',
+  MessageNotFound: 'There were no undeleted messages associated with conversation id',


May be better instead of "undeleted", maybe term it "There are no existing messages associated with conversation id" ?

Thanks, changed to no existing messages

… from `CORE#{messageId}` to just `CORE`

MohammadIqbalAD-NHS · 2024-03-13T14:10:47Z

src/services/database/dynamo-ehr-transfer-tracker.js

+      throw new Error('recordType has to be either Core or Fragment');
+    }
+    if (!inboundConversationId && !inboundMessageId) {
+      throw new Error('must be called with both conversationId and inboundMessageId');


could you replace "conversationId" with "inboundConversationId" for clarification.

Thanks, updated the error message

MohammadIqbalAD-NHS

I have reviewed. Once you have addressed last comments, let me know so I can approve. Good job on this, I'm impressed you got all this done in a week!

MohammadIqbalAD-NHS

Well Done!

joefong-nhs added 30 commits March 4, 2024 14:51

[PRMT-4565] store a working version of dynamodb localstack setup.

135ae52

[PRMT-4565] add basic test for dynamodb client

b71df2e

[PRMT-4565] minor change in config, small change to generate_secure_s…

d6747b9

…tring to address a mac specific problem

[PRMT-4565] Read dynamodb table name from SSM, change the table name …

41ec33a

…for local testing

[PRMDR-4565] Replace localstack with dynamodb-local for running dynam…

15e8a1c

…odb integration test

[PRMT-4565] Change timestamp format to include timezone offset

50afcd4

[PRMT-4566] add new version of updateFragmentAndCreateItsParts, start…

fee2d4f

… separating model and repository layer

[PRMT-4566] Config test setup and teardown

6f3fbc6

[PRMT-4566] Add option to query specific record type

9e332ee

[PRMT-4566] Add state enums

554bec9

[PRMT-4566] separate client and repository layer

1fac37f

[PRMT-4566] Reorganise new codes

68ef951

[PRMT-4566] Add terraform iam policy for accessing the ehr-transfer-t…

ff53598

…racker table

[PRMT-4566] Reorganise code, use CreatedAt instead of UpdatedAt to de…

590c11f

…termine latest record

[PRMT-4566] Migrate all tests in message-repository.integration.test.…

45bc5e3

…js to new dynamodb-based implementation

[PRMT-4566] Rename test files

047d5f1

[PRMT-4566] Continue migrating database methods and integration tests…

8189fac

…. Store current progress

[PRMT-4566] Migrating integrated test for more methods

8b7bfda

PRMT-4566 reorganise code

8a6cfd4

[PRMT-4566] Migrated all database-related integration tests. Store cu…

88892de

…rrent progress

[PRMT-4566] Rename tests files

afdacf5

[PRMT-4566] Rename methods to avoid import conflict, add deprecated m…

7471043

…ark and ticket number to old postgres related codes

[PRMT-4566] Rename methods

1226420

[PRMT-4566] minor change in wordings

d0b6a3d

[PRMT-4566] Correct dynamodb related env vars

85778d7

[PRMT-4566] Change DeletedAt field to store ttl time of 8 weeks later

54f9d94

[PRMT-4567] Migrated store-message-controller.js and its tests

9196cf5

[PRMT-4567] migrated message-location-controller.js and its tests

d78e32c

[PRMT-4567] migrated app integration tests for POST /messages endpoint

745f4ab

[PRMT-4567] rename test files

3f3cf46

joefong-nhs added 8 commits March 11, 2024 11:28

[PRMT-4568] Continue removing postgres-db related code

0bba6ef

[PRMT-4568] Remove redundant comments

dea9bd4

[PRMT-4568] Fix broken import of deprecated code

176538b

[PRMT-4568] Remove postgres related packages from dependencies

1289881

[PRMT-4568] Remove postgresdb from docker config, remove .sequelizerc

e2b731a

[PRMT-4568] Continue removing postgres-db related stuffs

025ccc8

[PRMT-4568] Remove postgres db related content from ./tasks (except t…

420a672

…erraform things)

[PRMT-4568] Run linter and formatter

1beb507

joefong-nhs force-pushed the PRMT-4568 branch from 5528313 to 1beb507 Compare March 11, 2024 13:33

[PRMT-4568] Remove commented out code

d1b5e7a

joefong-nhs changed the base branch from main to PRMT-4402 March 11, 2024 14:01

joefong-nhs added 2 commits March 11, 2024 14:10

[PRMT-4568] remove sequelize from package.json

3066b80

[PRMT-4568] Fix Dockerfile (remove references to deleted file)

9140ce1

MohammadIqbalAD-NHS reviewed Mar 12, 2024

View reviewed changes

joefong-nhs added 5 commits March 12, 2024 15:36

[PRMT-4568] remove db:migrate from package.json

e7181ad

[PRMT-4568] Use underscore instead of hyphen in terraform code

5540d9c

[PRMT-4568] Remove trailing comma

0871dcd

[PRMT-4568] Fix typo

d6a1d72

[PRMT-4568] fix typo (Outbound --> OUTBOUND) at TransferStatus

e2f7fd9

MohammadIqbalAD-NHS reviewed Mar 13, 2024

View reviewed changes

[PRMT-4568] Adapt to database schema change: sort key for core change…

432319e

… from `CORE#{messageId}` to just `CORE`

MohammadIqbalAD-NHS reviewed Mar 13, 2024

View reviewed changes

MohammadIqbalAD-NHS suggested changes Mar 13, 2024

View reviewed changes

[PRMT-4568] address PR comments

4a9db22

MohammadIqbalAD-NHS approved these changes Mar 14, 2024

View reviewed changes

MohammadIqbalAD-NHS merged commit fc47d7b into PRMT-4402 Mar 14, 2024

chrisbloe-nhse deleted the PRMT-4568 branch November 13, 2024 21:13

		// TODO: remove this duplicated `conversationIdFromEhrIn` field,
		// after updating ehr-out to use the field name "inboundConversationId"

Conversation

joefong-nhs commented Mar 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MohammadIqbalAD-NHS left a comment

Choose a reason for hiding this comment

Uh oh!

MohammadIqbalAD-NHS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants