-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api): clarify the notification steps creation #2995
Conversation
5cd2378
to
d138ad6
Compare
@@ -91,7 +91,10 @@ export class CreateNotificationTemplate { | |||
shouldStopOnFail: message.shouldStopOnFail, | |||
replyCallback: message.replyCallback, | |||
}); | |||
parentStepId = stepId; | |||
|
|||
if (stepId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to comply with setting _id as optional for MessageTemplateEntity.
ChangeEntityTypeEnum.MESSAGE_TEMPLATE, | ||
item._id | ||
); | ||
if (item._id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to comply with setting _id as optional for MessageTemplateEntity.
@@ -37,7 +37,10 @@ export class CreateMessageTemplate { | |||
actor: command.actor, | |||
}); | |||
|
|||
item = (await this.messageTemplateRepository.findById(item._id)) as MessageTemplateEntity; | |||
if (item?._id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to comply with setting _id as optional for MessageTemplateEntity.
@@ -51,7 +51,10 @@ export class PromoteNotificationTemplateChange { | |||
|
|||
return undefined; | |||
} | |||
step._templateId = oldMessage._id; | |||
|
|||
if (step?._templateId && oldMessage._id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to comply with setting _id as optional for MessageTemplateEntity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me 🥇
type: StepTypeEnum.TRIGGER, | ||
content: '', | ||
} as MessageTemplateEntity, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant to this PR but are you aware of why _layoutId is nullable and not optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retro-compatibility with the existing notificaiton templates after @ainouzgali and me implement the Layouts feature for email.
What change does this PR introduce?
Small refactor to aggregate the logic accordingly.
Why was this change needed?
Inside of the use case that was in charge on deciding if a digest step should be created or not we were doing more things, like filtering the active steps for the notification, adding extra complexity to that use case.
These changes should facilitate the vision if we are committing to decisions on creating or not digest steps before we should.
Other information (Screenshots)