Skip to content
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

fix(api,web): built in novu integrations for self hosted #5158

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

LetItRock
Copy link
Contributor

What change does this PR introduce?

Do not create Novu integrations when there are no credentials set for Novu Email or Novu SMS providers.
For self-hosted do not show the Novu providers on the Integrations page.
Missing E2E tests.

Why was this change needed?

Other information (Screenshots)

Screen.Recording.2024-02-05.at.10.44.17.mov

@LetItRock LetItRock self-assigned this Feb 5, 2024
Copy link

linear bot commented Feb 5, 2024

Copy link

netlify bot commented Feb 5, 2024

Deploy Preview for dev-web-novu failed.

Name Link
🔨 Latest commit 838a382
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/65cb725685dbbb00080c8e26

Comment on lines +85 to +90
if (
(command.providerId === SmsProviderIdEnum.Novu && !areNovuSmsCredentialsSet()) ||
(command.providerId === EmailProviderIdEnum.Novu && !areNovuEmailCredentialsSet())
) {
throw new BadRequestException(`Creating Novu integration for ${command.providerId} provider is not allowed`);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when creating the Novu integration for SMS or Email check if the environment variables for the provider credentials are set, if not throw an exception

Comment on lines +70 to +73
async execute(command: CreateNovuIntegrationsCommand): Promise<void> {
await this.createEmailIntegration(command);
await this.createSmsIntegration(command);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before: when some of the SMS or Email credentials were not set it didn't create any integration, for example even if Email credentials were set correctly

now: it does checks independently and will create Email/SMS integration if credentials are set

Comment on lines +26 to +29
if (IS_DOCKER_HOSTED) {
initialEmailProviders = emailProviders.filter((provider) => provider.id !== EmailProviderIdEnum.Novu);
initialSmsProviders = smsProviders.filter((provider) => provider.id !== SmsProviderIdEnum.Novu);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for self hosted - hide Novu Email and SMS integrations

@@ -29,7 +29,7 @@
"access": "public"
},
"dependencies": {
"@novu/stateless": "0.22.0",
"@novu/stateless": "0.23.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing version update

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very subtle note, can we convert this file name to novu-integrations.ts some systems might have issues not catching the capital letter when renamed. (Windows mostly)

@LetItRock LetItRock requested a review from scopsy February 6, 2024 10:23
Copy link
Member

@BiswaViraj BiswaViraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@LetItRock LetItRock merged commit bb104d5 into next Feb 13, 2024
26 of 30 checks passed
@LetItRock LetItRock deleted the nv-3424-fix-novu-providers-self-hosted branch February 13, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants