fix: proper error handling for missing subscriber on preference update #2754
Conversation
NV-1459 Add subscriber validation on /subscribers/:subscriberid/preferences
At the moment if the user is passing an incorrect subscriberid the server will return 500. we need to add validation + throwing API error with more informative message |
| const buildCommand = GetSubscriberTemplatePreferenceCommand.create({ | ||
| organizationId: job._organizationId, | ||
| subscriberId: job._subscriberId, | ||
| subscriberId: subscriber.subscriberId, |
There was a problem hiding this comment.
to make sure we only pass here the public subscriberId to avoid confusion
There was a problem hiding this comment.
This flow is too confusing! that's why I refactored it on the #2489 😀
| @@ -25,11 +26,16 @@ export class GetSubscriberTemplatePreference { | |||
|
|
|||
| async execute(command: GetSubscriberTemplatePreferenceCommand): Promise<ISubscriberPreferenceResponse> { | |||
| const activeChannels = await this.queryActiveChannels(command); | |||
| const subscriber = await this.subscriberRepository.findBySubscriberId(command.environmentId, command.subscriberId); | |||
| const subscriber = | |||
There was a problem hiding this comment.
To avoid a query when not needed
| @@ -174,11 +175,15 @@ export class SendMessage { | |||
| environmentId: job._environmentId, | |||
| }); | |||
|
|
|||
| const subscriber = await this.subscriberRepository.findById(job._subscriberId); | |||
There was a problem hiding this comment.
In this PR #2489 we added user's subscriberId to the job entity. I will add a not note on this PR to get the subscriber from the cache first.
There was a problem hiding this comment.
Awesome so we can fix it there later 🎉
There was a problem hiding this comment.
Yeah i left a note there so we wont forget 😅
| @@ -174,11 +175,15 @@ export class SendMessage { | |||
| environmentId: job._environmentId, | |||
| }); | |||
|
|
|||
| const subscriber = await this.subscriberRepository.findById(job._subscriberId); | |||
| if (!subscriber) throw new ApiException('Subscriber not found with id ' + job._subscriberId); | |||
There was a problem hiding this comment.
Btw I am not sure we need to add the exception here because if the flow got till here then we are after ProcessSubscriber usecase, in that case, the execute function already fetched/created a subscriber and passed it.
the possible exception we can get is when the user tries to update an existing subscriber through
/subscribers/:subscriberid/preferences
on the GetPreferences usecase flow.
wdyt?
There was a problem hiding this comment.
never mind i can see that you added a check on the get-subscriber-template-preference.usecase.ts. :)
...rs/usecases/get-subscriber-template-preference/get-subscriber-template-preference.usecase.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const subscriberPreference = await this.subscriberPreferenceRepository.findOne({ | ||
| _environmentId: command.environmentId, | ||
| _subscriberId: subscriber !== null ? subscriber._id : command.subscriberId, | ||
| _subscriberId: subscriber._id, |
…preference/get-subscriber-template-preference.usecase.ts Co-authored-by: George Djabarov <39195835+djabarovgeorge@users.noreply.github.com>
What change does this PR introduce?
Why was this change needed?
NV-1459
Other information (Screenshots)