Notification migration #2063

PooyaRaki · 2024-10-27T13:37:52Z

Summary

Add compatibility between Notification V1 and Notification V2. Integrate Notification V2 methods into Notification V1 to begin populating the database in advance of the data migration, ensuring clients can transition to V2 seamlessly without any changes on their end.

Changes

Integrate V2 methods within the V1 controller.
Refactor the V2 repository using TypeORM.

src/datasources/notifications/entities/__tests__/notification-devices.entity.db.builder.ts

src/datasources/notifications/entities/__tests__/notification-subscription.entity.db.builder.ts

src/datasources/notifications/entities/notification-devices.entity.db.ts

src/routes/notifications/v1/notifications.controller.ts

iamacook · 2024-11-04T14:04:50Z

src/routes/notifications/v2/entities/upsert-subscriptions.dto.entity.ts

+import { z } from 'zod';
+
+export const UpsertSubscriptionsDtoSafeSchema = z.object({
+  chainId: z.string(),


Suggested change

chainId: z.string(),

chainId: NumericStringSchema,

Done
b9eb8b9#diff-e9b850d8a5b41b96ae2d4047cc1f83829d53e4677a0bdaf23f0c17bd01ba606b

src/routes/notifications/v1/notifications.controller.ts

iamacook

All of my comments can be tackled in a follow up as they are validation focused. I would suggest we add schema tests for those implemented here as well.

src/datasources/notifications/entities/__tests__/notification-devices.entity.db.builder.ts

src/datasources/notifications/entities/__tests__/notification-subscription.entity.db.builder.ts

src/datasources/notifications/entities/notification-devices.entity.db.ts

src/datasources/notifications/entities/notification-subscription.entity.db.ts

src/datasources/notifications/entities/notification-devices.entity.db.ts

src/domain/notifications/v2/entities/upsert-subscriptions.dto.entity.ts

src/domain/notifications/v2/notifications.repository.integration.spec.ts

src/domain/notifications/v2/notifications.repository.ts

src/routes/notifications/v1/notifications.controller.spec.ts

hectorgomezv · 2024-11-06T10:05:06Z

migrations/1726752966034-notification.ts

@@ -8,22 +8,22 @@ export class Notification1726752966034 implements MigrationInterface {
      `CREATE TABLE "push_notification_devices" ("id" SERIAL NOT NULL, "device_type" character varying(255) NOT NULL, "device_uuid" uuid NOT NULL, "cloud_messaging_token" character varying(255) NOT NULL, "created_at" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updated_at" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), CONSTRAINT "device_uuid" UNIQUE ("device_uuid"), CONSTRAINT "PK_e387f5cc5b4f66d63804d596c64" PRIMARY KEY ("id"))`,


What do you think about formatting these queries to make them multiline? I think it might improve readability and make reviews easier.

I completely agree! However, I suggest creating a separate PR to enable automatic formatting(using tools like prettier) for these cases. What do you think?

I think it makes sense, thanks!

migrations/1726752966034-notification.ts

src/domain/notifications/v2/notifications.repository.spec.ts

src/domain/notifications/v2/test.notification.repository.interface.ts

hectorgomezv · 2024-11-06T14:21:47Z

src/routes/notifications/v1/notifications.controller.ts

+        const safeV2: {
+          authPayload: AuthPayload;
+          upsertSubscriptionsDto: UpsertSubscriptionsDto & {
+            safes: Array<UpsertSubscriptionsSafesDto>;
+            signature: `0x${string}`;
+          };


Could we extract this to a type, instead of typing it inline?

I totally agree. Since we usually define types inline, I did it this way to keep it consistent. Do you think we should consider creating an external type for it?

You should be able to extract this type from the controller using utility types, therefore keeping the controller as the SSOT.

As we discussed, I find heavy use of utility types makes the code extremely hard to read. However, I’d be happy to make the change if you could provide a code snippet as an example.

No strong opinion here, but I'd prefer to use utility types so we can have a SSOT as Aaron suggests 🙂 Feel free to keep the current implementation if you prefer.

src/routes/notifications/v1/notifications.controller.ts

src/routes/notifications/v1/entities/__tests__/safe-registration.builder.ts

src/routes/notifications/v1/entities/__tests__/register-device.dto.builder.ts

…s to v1

…rated to TypeORM.

… implementation.

…tests after enabling V2

…ository

…tionNotificationTypes

…ew tables

… notification v2

PooyaRaki force-pushed the notificationMigration branch from cf83376 to f53fa31 Compare October 27, 2024 17:16

PooyaRaki mentioned this pull request Oct 28, 2024

mock postgres database v1 in more tests [SW-403] #2064

Merged

PooyaRaki force-pushed the notificationMigration branch 4 times, most recently from 19e7c42 to a28bfab Compare November 4, 2024 12:07

PooyaRaki marked this pull request as ready for review November 4, 2024 12:16

PooyaRaki requested a review from a team as a code owner November 4, 2024 12:16

iamacook reviewed Nov 4, 2024

View reviewed changes

iamacook approved these changes Nov 6, 2024

View reviewed changes

PooyaRaki force-pushed the notificationMigration branch from bb16bc1 to 5fc9e0b Compare November 6, 2024 11:43

hectorgomezv reviewed Nov 6, 2024

View reviewed changes

PooyaRaki force-pushed the notificationMigration branch from 15978c2 to 73984fe Compare November 7, 2024 08:56

hectorgomezv self-requested a review November 7, 2024 15:48

hectorgomezv approved these changes Nov 7, 2024

View reviewed changes

iamacook approved these changes Nov 8, 2024

View reviewed changes

PooyaRaki added 14 commits November 8, 2024 12:12

rename database entity files to entity.db.ts, add notification v2 dto…

cbbf8fb

…s to v1

rewrite notification v2 using typeorm

7b565ac

call notification v2 from notification v1 to add compatibility

f347a2e

refactor notification hooks to use notification v2

3db5d5a

Mark Notification datasource as deprecated, as the logic has been mig…

5594b97

…rated to TypeORM.

Update Notification V2 controller tests to align with the new TypeORM…

b249ba4

… implementation.

adjust notification relations, add device_uuid to get safe subscription

aa860fc

test: Mock PushNotificationModule to resolve missing env vars in e2e …

cce2950

…tests after enabling V2

Remove legacy notification datasource, Add tests for notification rep…

8037a49

…ository

Add integration tests to notification repository

3cf4a9f

chore: refactor notification tests

71463bd

remove custom uuid type

8f0a1fe

Improve typing and validation schemas

94381b5

refactor notification controller tests

c02b89d

PooyaRaki added 12 commits November 8, 2024 12:12

Call notification v2 from v1 using Promise.all

26556ce

Add validation to notification controller v1, refactor insertSubscrip…

5dc6d43

…tionNotificationTypes

Add comment to insertSubscriptionNotificationTypes to make it more clear

ec8d937

Add mising created_at to NotificationSubscriptionSchema

5fd36fe

Adds UUIDSchema to NotificationDeviceSchema

cc6bdcb

Improve notification related types, refactor notification tests

1557f48

chore: Return device uuid directly from the database transaction handler

23c26c7

Get transactionRunner from the callback instead of the database service

bb5896f

fix notification repository unit tests

4c01a94

fix typos, moves notification repository mock to __tests__ folder

795c697

add row schema to notification subscription entity

5d89582

Add a new migration to clean up notification tables before creating n…

e6128ff

…ew tables

PooyaRaki force-pushed the notificationMigration branch from 037b635 to e6128ff Compare November 8, 2024 11:12

Check whether the notification feature flag is enabled before calling…

855d848

… notification v2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notification migration #2063

Notification migration #2063

PooyaRaki commented Oct 27, 2024

iamacook Nov 4, 2024

PooyaRaki Nov 5, 2024

iamacook left a comment

hectorgomezv Nov 6, 2024

PooyaRaki Nov 6, 2024

hectorgomezv Nov 7, 2024

hectorgomezv Nov 6, 2024

PooyaRaki Nov 6, 2024

iamacook Nov 7, 2024

PooyaRaki Nov 7, 2024

hectorgomezv Nov 7, 2024

		@@ -8,22 +8,22 @@ export class Notification1726752966034 implements MigrationInterface {
		`CREATE TABLE "push_notification_devices" ("id" SERIAL NOT NULL, "device_type" character varying(255) NOT NULL, "device_uuid" uuid NOT NULL, "cloud_messaging_token" character varying(255) NOT NULL, "created_at" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), "updated_at" TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT now(), CONSTRAINT "device_uuid" UNIQUE ("device_uuid"), CONSTRAINT "PK_e387f5cc5b4f66d63804d596c64" PRIMARY KEY ("id"))`,

Notification migration #2063

Are you sure you want to change the base?

Notification migration #2063

Conversation

PooyaRaki commented Oct 27, 2024

Summary

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment