-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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-service): get subscriber preferences v2 endpoint #7613
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@novu/client
@novu/headless
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/shared
commit: |
|
||
export interface IGetSubscriberPreferencesResponseDto { | ||
global: { | ||
enabled: boolean; |
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.
Fun fact:
we allow enabled
in update DTO, to disable all global preferences as I understand, but we never use it anywhere:
export type UpdateSubscriberGlobalPreferencesRequestDto = {
/**
* Enable or disable the subscriber global preferences.
*/
enabled?: boolean | undefined;
/**
* The subscriber global preferences for every ChannelTypeEnum.
*/
preferences?: Array<ChannelPreference> | undefined;
};
we always return enabled: true
so you can't in fact disable all preferences globally.
Line 110 in 368050c
const enabled = true; |
export interface ISubscriberWorkflowPreferenceResponse extends IPreferenceResponse { | ||
workflow: ITemplateConfiguration; | ||
level: PreferenceLevelEnum.TEMPLATE; | ||
} | ||
|
||
export interface IWorkflow extends Omit<ITemplateConfiguration, '_id'> { | ||
id: string; | ||
} | ||
export interface ISubscriberPreferences { | ||
level: PreferenceLevelEnum; | ||
workflow?: IWorkflow; | ||
enabled: boolean; | ||
channels: IPreferenceChannels; | ||
overrides?: IPreferenceOverride[]; | ||
} | ||
|
||
export interface IPreferenceResponse { |
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 used so I removed it.
return plainToInstance(GetSubscriberPreferencesDto, { | ||
global: globalPreference, | ||
workflows: workflowPreferences, | ||
}); |
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.
I think we should serialize response to DTO like this in all new endpoints, maybe better in controller instead of usecase - it ensures response is validated and transformed to what is specified in DTO class.
It can be also setup globally on all controlles so this line is not needed.
https://docs.nestjs.com/techniques/serialization#overview
Between usecases, I would left just regular types.
I think this would lead to generally cleaner experience.
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.
I'm not sure if that is needed because everything that comes from the DAL layer should already be valid. Otherwise, it means that we have a corrupted state in DB.
But I would like to hear some other opinions.
BTW plainToInstance
can be done on the base class that we can extend from, so we don't need to do it every time.
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.
What I mean is the DTO that comes as a response from API, DAL layer is business logic, this is to better illustrate - we dont serialize and we think we do, in order for ClassSerializer decorator to work, you need to return instance from a controller (or it can be also automatized by other decorator) - but we dont do it correctly.
summary: 'Get subscriber preferences', | ||
description: 'Get subscriber preferences', | ||
}) | ||
async getSubscriberPreferences( |
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.
Do we want also to define a custom sdk method name? I'm wondering how it will name it by default.
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.
I've named it retrieve
, I think thats the convention
return plainToInstance(GetSubscriberPreferencesDto, { | ||
global: globalPreference, | ||
workflows: workflowPreferences, | ||
}); |
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.
I'm not sure if that is needed because everything that comes from the DAL layer should already be valid. Otherwise, it means that we have a corrupted state in DB.
But I would like to hear some other opinions.
BTW plainToInstance
can be done on the base class that we can extend from, so we don't need to do it every time.
} | ||
|
||
private mapToWorkflowPreference(subscriberWorkflowPreference: ISubscriberPreferenceResponse): WorkflowPreferenceDto { | ||
const { preference, template } = subscriberWorkflowPreference; |
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 should have workflow
instead of template
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 is a return type from already existing getSubscriberPreference
usecase which is reused in other places across the app.
throw new NotFoundException( | ||
`Subscriber ${command.subscriberId} not found`, | ||
); |
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.
did you verify that this usecase is not used in worker?
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.
Yes - from what I see, worker is using just GetSubscriberPreference
usecase, but even for that usecase I can't find any use inside worker - I will check if we can remove it possibly in next PR.
What changed? Why was the change needed?
TODO: