fix(api): Subscriber deletion side-effects #6872

SokratisVidros · 2024-11-06T15:00:01Z

What changed? Why was the change needed?

Clear subscribers along with all their related data and their cached entries

SokratisVidros · 2024-11-06T15:00:49Z

apps/api/src/app/subscribers/e2e/remove-subscriber.e2e.ts

-    )?.[0];
-
-    expect(deletedSubscriber.deleted).to.equal(true);
+    const subscriber = await subscriberRepository.findBySubscriberId(session.environment._id, '123');


Simplify the E2E so that the findDeleted can be removed.

SokratisVidros · 2024-11-06T15:01:24Z

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.spec.ts

@@ -51,10 +47,9 @@ describe('Remove Subscriber', function () {
          organizationId: session.organization._id,
        })
      );
-      throw new Error('Should not reach here');
+      expect(true, 'Should never reach this statement').to.be.false;


A small refactoring without throwing an error will trigger the catch block again and will provide a better message for the test results.

SokratisVidros · 2024-11-06T15:01:58Z

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.usecase.ts

        }),
-      });
+      }),
+      this.invalidateCache.invalidateQuery({


Clear inbox entries from cache

SokratisVidros · 2024-11-06T15:02:17Z

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.usecase.ts

-        _environmentId: subscriber._environmentId,
-        _organizationId: subscriber._organizationId,
-        subscriberId: subscriber.subscriberId,
+    await this.subscriberRepository.withTransaction(async () => {


Delete all related records atomically. Postgres, I miss you!

SokratisVidros · 2024-11-06T15:02:38Z

libs/dal/src/repositories/subscriber/subscriber.repository.ts

@@ -157,55 +157,19 @@ export class SubscriberRepository extends BaseRepository<SubscriberDBModel, Subs
    );
  }

-  async delete(query: SubscriberDeleteQuery) {


Clean ups for unused methods

SokratisVidros · 2024-11-06T15:03:40Z

libs/dal/src/repositories/preferences/preferences.repository.ts

@@ -28,13 +28,6 @@ export class PreferencesRepository extends BaseRepository<PreferencesDBModel, Pr
    return this.mapEntity(item);
  }

-  async delete(query: PreferencesQuery) {
-    const item = await this.findOne({ _id: query._id, _environmentId: query._environmentId });


There is no need for an extra query when trying to delete a record. The number of affected rows returned by mongoose indicates if the record was found or not.

SokratisVidros · 2024-11-06T15:04:25Z

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.usecase.ts

+      });
+    });
+
+    if (deletedSubscriberCount === 0) {


Usually, I prefer always returning a 2xx even if the record is already deleted but since this is a breaking change for the API, I preserved the previous 404 error.

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.spec.ts

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.usecase.ts

netlify · 2024-11-06T15:49:37Z

✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name	Link
🔨 Latest commit	`4e1a6f1`
🔍 Latest deploy log	https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/672dc87379a3d30008b74415
😎 Deploy Preview	https://deploy-preview-6872--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

1. Simplify the delete method to delete a subscriber without fetching them first. 2. Remove unused methods

libs/application-generic/src/usecases/upsert-preferences/upsert-preferences.usecase.ts

rifont

💯

…t-preferences.usecase.ts Co-authored-by: Richard Fontein <32132657+rifont@users.noreply.github.com>

SokratisVidros requested review from scopsy, rifont and djabarovgeorge November 6, 2024 15:00

SokratisVidros self-assigned this Nov 6, 2024

SokratisVidros temporarily deployed to Linting November 6, 2024 15:00 — with GitHub Actions Inactive

github-actions bot added @novu/api @novu/dal labels Nov 6, 2024

SokratisVidros commented Nov 6, 2024

View reviewed changes

rifont reviewed Nov 6, 2024

View reviewed changes

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.spec.ts Outdated Show resolved Hide resolved

SokratisVidros force-pushed the subscriber_deletion_sideeffects branch from 1ace52f to c544a89 Compare November 6, 2024 15:18

SokratisVidros temporarily deployed to Linting November 6, 2024 15:18 — with GitHub Actions Inactive

SokratisVidros changed the title ~~Subscriber deletion sideeffects~~ fix(api): Subscriber deletion side-effects Nov 6, 2024

djabarovgeorge reviewed Nov 6, 2024

View reviewed changes

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.usecase.ts Outdated Show resolved Hide resolved

djabarovgeorge reviewed Nov 6, 2024

View reviewed changes

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.usecase.ts Outdated Show resolved Hide resolved

djabarovgeorge reviewed Nov 6, 2024

View reviewed changes

apps/api/src/app/subscribers/usecases/remove-subscriber/remove-subscriber.usecase.ts Show resolved Hide resolved

SokratisVidros force-pushed the subscriber_deletion_sideeffects branch from c544a89 to e5f8b6c Compare November 6, 2024 15:46

SokratisVidros temporarily deployed to Linting November 6, 2024 15:46 — with GitHub Actions Inactive

SokratisVidros temporarily deployed to Linting November 6, 2024 15:58 — with GitHub Actions Inactive

SokratisVidros temporarily deployed to Linting November 6, 2024 15:59 — with GitHub Actions Inactive

SokratisVidros force-pushed the subscriber_deletion_sideeffects branch from cc62ac9 to c75065f Compare November 7, 2024 13:41

SokratisVidros temporarily deployed to Linting November 7, 2024 13:41 — with GitHub Actions Inactive

SokratisVidros force-pushed the subscriber_deletion_sideeffects branch from c75065f to ce822b4 Compare November 7, 2024 20:56

SokratisVidros temporarily deployed to Linting November 7, 2024 20:56 — with GitHub Actions Inactive

SokratisVidros added 5 commits November 7, 2024 23:57

fix(api): Clear Inbox and NC cached entries on subscriber deletion

fe17727

chore(api): Simplify subscriber repository

90afe2d

1. Simplify the delete method to delete a subscriber without fetching them first. 2. Remove unused methods

feat(dal): Expose Mongoose transactions

5f74fc8

fix(api): Remove subscribers along with their related entities

f5fcff5

fixup! fix(api): Remove subscribers along with their related entities

98bf829

SokratisVidros force-pushed the subscriber_deletion_sideeffects branch from ce822b4 to 98bf829 Compare November 7, 2024 22:20

SokratisVidros temporarily deployed to Linting November 7, 2024 22:20 — with GitHub Actions Inactive

SokratisVidros requested review from rifont and djabarovgeorge November 7, 2024 22:32

rifont reviewed Nov 8, 2024

View reviewed changes

libs/application-generic/src/usecases/upsert-preferences/upsert-preferences.usecase.ts Outdated Show resolved Hide resolved

rifont approved these changes Nov 8, 2024

View reviewed changes

Update libs/application-generic/src/usecases/upsert-preferences/upser…

4e1a6f1

…t-preferences.usecase.ts Co-authored-by: Richard Fontein <32132657+rifont@users.noreply.github.com>

SokratisVidros temporarily deployed to Linting November 8, 2024 08:14 — with GitHub Actions Inactive

SokratisVidros had a problem deploying to Linting November 8, 2024 08:14 — with GitHub Actions Failure

SokratisVidros merged commit 5a0efd7 into next Nov 8, 2024
9 of 10 checks passed

SokratisVidros deleted the subscriber_deletion_sideeffects branch November 8, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): Subscriber deletion side-effects #6872

fix(api): Subscriber deletion side-effects #6872

SokratisVidros commented Nov 6, 2024

SokratisVidros Nov 6, 2024

SokratisVidros Nov 6, 2024 •

edited

Loading

SokratisVidros Nov 6, 2024

SokratisVidros Nov 6, 2024 •

edited

Loading

SokratisVidros Nov 6, 2024

SokratisVidros Nov 6, 2024

SokratisVidros Nov 6, 2024 •

edited

Loading

netlify bot commented Nov 6, 2024 •

edited

Loading

rifont left a comment

fix(api): Subscriber deletion side-effects #6872

fix(api): Subscriber deletion side-effects #6872

Conversation

SokratisVidros commented Nov 6, 2024

What changed? Why was the change needed?

SokratisVidros Nov 6, 2024

Choose a reason for hiding this comment

SokratisVidros Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

SokratisVidros Nov 6, 2024

Choose a reason for hiding this comment

SokratisVidros Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

SokratisVidros Nov 6, 2024

Choose a reason for hiding this comment

SokratisVidros Nov 6, 2024

Choose a reason for hiding this comment

SokratisVidros Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

netlify bot commented Nov 6, 2024 • edited Loading

✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!

rifont left a comment

Choose a reason for hiding this comment

SokratisVidros Nov 6, 2024 •

edited

Loading

SokratisVidros Nov 6, 2024 •

edited

Loading

SokratisVidros Nov 6, 2024 •

edited

Loading

netlify bot commented Nov 6, 2024 •

edited

Loading