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

Add IP Pool Name In Integration #3647

Merged
merged 2 commits into from
Jun 25, 2023
Merged

Conversation

djabarovgeorge
Copy link
Contributor

What change does this PR introduce?

Add ip pool name into the send grid integration.

Why was this change needed?

in order to provide the user the ability to add/update it.

Other information (Screenshots)

i refactored(moved to shared) the ICredentials by mistake we had too many duplications of it mostly mine fult.
i created an ICredentials in shared.
deleted some of the duplications.
currently created type of the interface because they were the same, it this type will need to change we change to update it to interface and implement the shared ICredentials.

@@ -115,4 +116,24 @@ export class CredentialsDto {
@IsString()
@IsOptional()
webhookUrl?: string;

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 attributes that i located due the implements ICredentials (in a hacky way)


export class CredentialsDto {
export class CredentialsDto implements ICredentials {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this addition implements:

  • is not helping with the compilation time with missing attributes because all of the attributes in ICredentials are optional.
  • it will alert in case we provide an invalid type.

@@ -328,32 +329,6 @@ export interface IIntegratedProvider {
novu?: boolean;
}

export interface ICredentials {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧹

@@ -152,34 +153,6 @@ export interface IIntegratedProvider {
novu?: boolean;
}

export interface ICredentials {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧹


import type { EnvironmentId } from '../environment';
import type { OrganizationId } from '../organization';
import { ChangePropsValueType } from '../../types/helpers';

export interface ICredentials {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧹

@@ -1,30 +1,6 @@
export interface ICredentialsDto {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧹

@@ -59,4 +30,6 @@ export class IntegrationEntity {
deletedBy: string;
}

export type ICredentialsEntity = ICredentials;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

was not sure what to call this one :/
because it is basically a type of interface, i left the I for now.
any thoughts about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to keep the "Entity" separation so we will have it in code. because one can change while the other one may not.

@@ -1,4 +1,4 @@
import { ICredentials } from '@novu/dal';
import { ICredentials } from '@novu/shared';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that the shared one will do the work in the provider's packages.

Base automatically changed from add-id-pool-override to next June 22, 2023 13:38
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

The naming and location of some of the interfaces can be done in the future. This PR is big enough already.
🌟

@djabarovgeorge djabarovgeorge added this pull request to the merge queue Jun 25, 2023
Merged via the queue into next with commit 8d2a5fc Jun 25, 2023
31 checks passed
@djabarovgeorge djabarovgeorge deleted the add-ip-pool-name-in-integration branch June 25, 2023 07:07
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.

2 participants