feat(api-service): inbox bulk update preferences #8292
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
| @@ -313,10 +316,26 @@ export class InboxController { | |||
| } | |||
|
|
|||
| @UseGuards(AuthGuard('subscriberJwt')) | |||
| @Patch('/preferences/:workflowId') | |||
| @Patch('/preferences/bulk') | |||
There was a problem hiding this comment.
the order matters, this one should be before the PATCH /preferences/:workflowIdOrIdentifier, otherwise the latter will handle the call
There was a problem hiding this comment.
Let's maybe add an in-code comment for this?
| async updateWorkflowPreference( | ||
| @SubscriberSession() subscriberSession: SubscriberEntity, | ||
| @Param('workflowId') workflowId: string, | ||
| @Param('workflowIdOrIdentifier') workflowIdOrIdentifier: string, |
There was a problem hiding this comment.
improved this one to allow using the workflow Mongo id or identifier
| const allWorkflowIds = command.preferences.map((preference) => preference.workflowIdOrInternalId); | ||
| const workflowInternalIds = allWorkflowIds.filter((id) => BaseRepository.isInternalId(id)); | ||
| const workflowIdentifiers = allWorkflowIds.filter((id) => !BaseRepository.isInternalId(id)); | ||
|
|
||
| const dbWorkflows = await this.notificationTemplateRepository.find({ | ||
| _environmentId: command.environmentId, | ||
| $or: [{ _id: { $in: workflowInternalIds } }, { 'triggers.identifier': { $in: workflowIdentifiers } }], | ||
| }); |
There was a problem hiding this comment.
split the mongo ids from the identifiers, this ensures that we don't get the mongo casting error to ObjectId
| const invalidWorkflowIds = allWorkflowIds.filter((id) => !allValidWorkflowsMap.has(id)); | ||
| if (invalidWorkflowIds.length > 0) { | ||
| throw new NotFoundException(`Workflows with ids: ${invalidWorkflowIds.join(', ')} not found`); | ||
| } |
There was a problem hiding this comment.
throw 404 if some of the workflows are not found
| const criticalWorkflows = dbWorkflows.filter((workflow) => workflow.critical); | ||
| if (criticalWorkflows.length > 0) { | ||
| const criticalWorkflowIds = criticalWorkflows.map((workflow) => workflow._id); | ||
| throw new BadRequestException(`Critical workflows with ids: ${criticalWorkflowIds.join(', ')} cannot be updated`); | ||
| } |
There was a problem hiding this comment.
we don't allow updating preferences for critical workflows
| const workflowPreferencesMap = new Map<string, BulkUpdatePreferenceItemDto>(); | ||
| for (const preference of command.preferences) { | ||
| const workflow = allValidWorkflowsMap.get(preference.workflowIdOrInternalId); | ||
| if (workflow) { | ||
| workflowPreferencesMap.set(workflow._id, preference); | ||
| } | ||
| } |
There was a problem hiding this comment.
deduplicate workflows; the user can send in the request either id or identifier, but both can be pointing to the same mongo document, so we would like to deduplicate in that case
| const workflow = await this.getWorkflowByIdsUsecase.execute( | ||
| GetWorkflowByIdsCommand.create({ | ||
| environmentId: command.environmentId, | ||
| organizationId: command.organizationId, | ||
| workflowIdOrInternalId: command.workflowIdOrIdentifier, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
use the existing usecase to find by id or identifier
There was a problem hiding this comment.
Looks good!
One note I have, is that I feel like the workflowIdOrInternalId is not really needed, and I would just stick to the workflowId (not the internal one) as it seems like it can reduce some of the complexity around the implementation.
I'm not too strong on this, but do we have any other place that follows this pattern?
| export class BulkUpdatePreferenceItemDto extends UpdatePreferencesRequestDto { | ||
| @IsDefined() | ||
| @IsString() | ||
| readonly workflowIdOrInternalId: string; |
There was a problem hiding this comment.
Do we have this pattern in other places? wonder if it's better to just stick to workflowId, it's hard for me to see if anyone will ever user the internal id for this
There was a problem hiding this comment.
@scopsy the workflow controller uses workflowId as the path param, so I'll stick to that name
| @@ -313,10 +316,26 @@ export class InboxController { | |||
| } | |||
|
|
|||
| @UseGuards(AuthGuard('subscriberJwt')) | |||
| @Patch('/preferences/:workflowId') | |||
| @Patch('/preferences/bulk') | |||
There was a problem hiding this comment.
Let's maybe add an in-code comment for this?
| @@ -313,10 +316,26 @@ export class InboxController { | |||
| } | |||
|
|
|||
| @UseGuards(AuthGuard('subscriberJwt')) | |||
| @Patch('/preferences/:workflowId') | |||
| @Patch('/preferences/bulk') | |||
There was a problem hiding this comment.
Any specific reason for PATCH vs POST? because of the partial preference object?
There was a problem hiding this comment.
yes it's partial update and to align with the existing single update
What changed? Why was the change needed?
Inbox bulk update preferences endpoint:
PATCH /inbox/preferences/bulk.Improved the current update single preferences endpoint to accept a workflow Mongo ID or identifier.
Screenshots