-
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): homogenize trigger event use cases and fix tech debt #2885
Conversation
@@ -56,7 +54,6 @@ export const USE_CASES = [ | |||
SendMessageDelay, | |||
SendTestEmail, | |||
MapTriggerRecipients, | |||
MapTriggerRecipientsCommand, |
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.
This was a mistake I introduced time ago.
export class TestSendMessageCommand extends EnvironmentWithUserCommand { | ||
@IsDefined() | ||
contentType: 'customHtml' | 'editor'; | ||
@IsDefined() | ||
payload: any; // eslint-disable-line @typescript-eslint/no-explicit-any | ||
@IsDefined() | ||
@IsString() | ||
subject: string; | ||
preheader?: string; | ||
senderName?: string; | ||
@IsDefined() | ||
content: string | IEmailBlock[]; | ||
@IsDefined() | ||
to: string | string[]; | ||
@IsOptional() | ||
@IsString() | ||
layoutId?: string | null; | ||
} |
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.
Moving to its own file.
@@ -21,7 +23,7 @@ export class SendTestEmail { | |||
private getDecryptedIntegrationsUsecase: GetDecryptedIntegrations | |||
) {} | |||
|
|||
public async execute(command: TestSendMessageCommand) { | |||
public async execute(command: SendTestEmailCommand) { |
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.
We had a mixing naming style for this use case and command. Homogenised.
@@ -1,5 +1,11 @@ | |||
import type { BuilderFieldOperator } from './builder.types'; | |||
|
|||
export enum TimeOperatorEnum { |
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.
Added for tighter validation.
We could reuse it in other places we use these time strings. For example, DigestUnitEnum.
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.
Loved it 👍🏻
63819aa
to
3e88330
Compare
@@ -458,7 +460,7 @@ async function findAsync<T>(array: T[], predicate: (t: T) => Promise<boolean>): | |||
} | |||
|
|||
async function filterAsync<T>(arr: T[], callback: (item: T) => Promise<boolean>): Promise<T[]> { | |||
const fail = Symbol(); | |||
const fail = Symbol('Filter Async failure'); |
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.
DeepSource flagged this.
preheader?: string; | ||
senderName?: string; |
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.
Should we add IsOptional() here?
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, added small comment there
What change does this PR introduce?
Homogenise the Trigger Event module use cases and fixes some tech debt related.
Also moves MessageMatcher to its own location as being an use case and not being used by TriggerEvent usecase.
Why was this change needed?
Tech debt.
Other information (Screenshots)