Nv 2215 delete messagetemplates of deleted templates #3524

BiswaViraj · 2023-05-29T05:28:28Z

What change does this PR introduce?

Delete the message templates and add them to the changes upon deletion of the Notification template

Why was this change needed?

Other information (Screenshots)

linear · 2023-05-29T05:28:31Z

NV-2215 delete messageTemplates of deleted templates

Why? (Context)

Within each Template, we have steps. each step has messageTemplate (content). When the user delete the templates, message templates are not deleted.

What?

As message templates are still there without the parent template, in the case of an in-app step, the feed is associated with the message template, User is not able to delete the feed

Definition of Done

message templates should be deleted with the deletion of the template
user should be able to delete the feed
to check if the feed can be deleted in there are some sent notifications of this template.
Do we need to delete previously sent notifications of this template also if the user delete the template
A change should be generated to promote to production

BiswaViraj · 2023-05-29T05:41:12Z

apps/web/src/pages/templates/components/TemplateEditorFormProvider.tsx

+        reset({
+          ...form,
+          steps: response.steps,
+        });


This fixes the issue where multiple duplicate message templates were being created while updating the Notification template from the UI, also fixes the deletion of feed error.

Steps to reproduce(without this code change):

Create a new workflow

add an in-app step, fill the details

within the in-app editor screen, click update button

go back, and add another in-app step

keep the second in-app editor screen open, click update

do some minor changes in the content, click update

Check DB, there will be multiple copies of the 2nd step.

@LetItRock You added this for some digest related issue. can you please confirm if this change works for that issue?

ainouzgali

Great job @BiswaViraj🤩
I just have a few concerns:

Let's do a soft delete of the message template like we do for the other repositories.
In promoting the notification template change it calls the promote message template, which currently has no delete behavior.
When removing a step - not in the delete template - what do you think of also deleting the message template? I believe we are just updating the notification template now.
Lets add a couple of tests for the promotion of notification template promotes deletion of the message templates + able to delete a feed.

ainouzgali · 2023-05-29T06:09:14Z

apps/api/src/app/notification-template/e2e/delete-notification-template.e2e.ts

@@ -139,6 +145,47 @@ describe('Delete notification template by id - /notification-templates/:template

    expect(response.body.message).to.contains('Could not find workflow with id');
  });
+
+  it('should delete the notification template along with the message templates', async function () {


What do you think about also adding a test of deleting a notification template that has a feed assigned to it and then see that the feed can be deleted?

Agree, that will be a good addition, will add it.

ainouzgali · 2023-05-29T06:16:52Z

...src/app/message-template/usecases/delete-message-template/delete-message-template.usecase.ts

+          item: template,
+          type: ChangeEntityTypeEnum.MESSAGE_TEMPLATE,
+          parentChangeId: command.parentChangeId,
+          changeId: MessageTemplateRepository.createObjectId(),


I believe here changeId should get the value from changeRepository.getChangeId because then it will aggregate all current changes made to that message template into one.

p-fernandez

🌟

p-fernandez · 2023-05-30T09:05:27Z

...src/app/message-template/usecases/delete-message-template/delete-message-template.usecase.ts

+      const template = await this.messageTemplateRepository.findById(command.messageTemplateId, command.environmentId);
+      if (!template) {
+        throw new NotFoundException(`Message Template with id ${command.messageTemplateId} not found`);
+      }


As a tech debt we should create an use case to find the message template to handle this logic as it can be reused in UpdateMessageTemplate.

p-fernandez · 2023-05-30T09:08:36Z

apps/api/src/app/notification-template/e2e/delete-notification-template.e2e.ts

+    const deletedIntegration = (
+      await notificationTemplateRepository.findDeleted({ _environmentId: session.environment._id, _id: template._id })
+    )[0];


Suggested change

const deletedIntegration = (

await notificationTemplateRepository.findDeleted({ _environmentId: session.environment._id, _id: template._id })

)[0];

const [deletedIntegration] = await notificationTemplateRepository.findDeleted({ _environmentId: session.environment._id, _id: template._id });

Same vibes. 🙂
Not to address it as it is stylistic and we have no defined preference for any.

p-fernandez · 2023-05-30T09:10:35Z

...ation-template/usecases/delete-notification-template/delete-notification-template.usecase.ts

+        await this.deleteMessageTemplate.execute(
+          DeleteMessageTemplateCommand.create({
+            organizationId: command.organizationId,
+            environmentId: command.environmentId,
+            userId: command.userId,
+            messageTemplateId: step._templateId,
+            parentChangeId: parentChangeId,


Do we want to stop the execution of the notification template deletion if any of the message template deletions fail?

…mplate

BiswaViraj requested review from LetItRock, p-fernandez and djabarovgeorge May 29, 2023 05:28

BiswaViraj self-assigned this May 29, 2023

github-actions bot added @novu/api @novu/web labels May 29, 2023

BiswaViraj force-pushed the nv-2215-delete-messagetemplates-of-deleted-templates branch from 5e07a28 to c24ada1 Compare May 29, 2023 05:31

BiswaViraj commented May 29, 2023

View reviewed changes

ainouzgali reviewed May 29, 2023

View reviewed changes

p-fernandez approved these changes May 30, 2023

View reviewed changes

BiswaViraj force-pushed the nv-2215-delete-messagetemplates-of-deleted-templates branch from c24ada1 to e017019 Compare May 30, 2023 14:57

github-actions bot added the @novu/dal label May 30, 2023

BiswaViraj added 12 commits June 10, 2023 13:19

feat: delete message template usecase

9c9b311

feat: delete child message templates upon deletion of notification te…

a7501b0

…mplate

test: e2e test cases for deletion of templates

7236278

fix: steps duplication which lead to orphan message templates

18b280a

refactor: remove unused imports

ae8bb20

feat: add delete softmodel

c3bf44c

feat: handle deletion of template while promoting

f02cdae

feat: soft delete

69032b8

feat: delete removed message templates during update

f3eacfd

tests:added more tests

5e56b28

fix: tests

c27f69d

fix: failing tests and stop notification deletion on error

705b3e7

BiswaViraj force-pushed the nv-2215-delete-messagetemplates-of-deleted-templates branch from b962087 to 705b3e7 Compare June 10, 2023 09:19

fix: typo

5a4cf36

BiswaViraj added this pull request to the merge queue Jun 10, 2023

Merged via the queue into next with commit f4a0481 Jun 10, 2023

BiswaViraj deleted the nv-2215-delete-messagetemplates-of-deleted-templates branch June 10, 2023 09:39

djabarovgeorge mentioned this pull request Aug 16, 2023

patch : remove rows only when steps are truthy #3980

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nv 2215 delete messagetemplates of deleted templates #3524

Nv 2215 delete messagetemplates of deleted templates #3524

BiswaViraj commented May 29, 2023

linear bot commented May 29, 2023

Why? (Context)

What?

Definition of Done

BiswaViraj May 29, 2023 •

edited

Loading

ainouzgali left a comment

ainouzgali May 29, 2023

BiswaViraj May 29, 2023

ainouzgali May 29, 2023

p-fernandez left a comment

p-fernandez May 30, 2023

p-fernandez May 30, 2023

p-fernandez May 30, 2023

Nv 2215 delete messagetemplates of deleted templates #3524

Nv 2215 delete messagetemplates of deleted templates #3524

Conversation

BiswaViraj commented May 29, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented May 29, 2023

Why? (Context)

What?

Definition of Done

BiswaViraj May 29, 2023 • edited Loading

Choose a reason for hiding this comment

ainouzgali left a comment

Choose a reason for hiding this comment

ainouzgali May 29, 2023

Choose a reason for hiding this comment

BiswaViraj May 29, 2023

Choose a reason for hiding this comment

ainouzgali May 29, 2023

Choose a reason for hiding this comment

p-fernandez left a comment

Choose a reason for hiding this comment

p-fernandez May 30, 2023

Choose a reason for hiding this comment

p-fernandez May 30, 2023

Choose a reason for hiding this comment

p-fernandez May 30, 2023

Choose a reason for hiding this comment

BiswaViraj May 29, 2023 •

edited

Loading