-
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): Endpoint for canceling events in bulk #6059
base: next
Are you sure you want to change the base?
Conversation
@hiteshwadhwani sorry if i may be wrong, but is it also required to edit the node-packages, like here: https://github.com/novuhq/novu/blob/next/packages/node/src/lib/events/events.ts or is this auto-generated afterwards? |
I didn't update any node-package |
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.
Thank you for your contribution and our apologies for the delay in reviewing this.
Please address the comments and ensure the metadata.ts
is fully up to date by running the API in dev mode (pnpm start:api
).
export class ProcessBulkCancel { | ||
constructor(private cancelDelayed: CancelDelayed) {} | ||
|
||
async execute(command: ProcessBulkCancelCommand) { |
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.
☑ chore: Please add a return typing to this function using the Dto to fully enforce the use-case boundary with type-safety.
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.
done
@ThrottlerCost(ApiRateLimitCostEnum.BULK) | ||
@Post('/trigger/bulk-cancel') | ||
@ApiOkResponse({ | ||
type: TriggerBulkCancelResponseDto, |
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.
⚠ issue: The new endpoint takes an array of transactionId
s, therefore the endpoint should produce an array of cancel response Dtos, one response Dto per transactionId
. We can achieve specification of this with Nestjs swagger plugin by wrapping the type
field with square braces.
type: TriggerBulkCancelResponseDto, | |
type: [TriggerBulkCancelResponseDto], |
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.
done
Regarding updates to the Node & other language SDKs. The current SDKs are not auto-generated and require manual updates to support new API endpoints. If you intend to use this new bulk cancel endpoint over SDK, please make the necessary changes to your SDK language you are using (if you are using Node, include the necessary changes in this PR, or for other languages raise another PR in the relevant repository). If you do not intend to use this new endpoint via SDK, we can allow this change to NOT have an accompanying SDK update as we plan to release auto-generated SDKs for all top languages (Node, Python, Go, Java, PHP) within the next month and we can defer the update to that time. |
…el events in bulk
I have added a method triggerBulkCancel in node SDK to cancel events in bulk and addressed the PR comments @rifont |
What changed? Why was the change needed?
This PR adds the feature to cancel events in bulk #5818
Endpoint details -
URL: v1/events/trigger/bulk-cancel
Method: POST
Request body: list of transaction Ids (Example: ["123134234", "12234234"])
Response: {
transactionId: string,
success: boolean,
error: string[]
}[]