feat(api): fail inactive notification trigger #7347
Conversation
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
✅ 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. |
| @@ -405,6 +421,10 @@ export class ParseEventRequest { | |||
| private isBlockedEmail(email: string): boolean { | |||
| return BLOCKED_DOMAINS.some((domain) => email.includes(domain)); | |||
| } | |||
|
|
|||
| private isGracefulTriggerAllowed(source?: string): boolean { | |||
| return source === TriggerSourceEnum.DASHBOARD; | |||
There was a problem hiding this comment.
@djabarovgeorge, why do we need to check the source for this? I suggest just using the status for now.
There was a problem hiding this comment.
please see the description of the PR
At the same time, to improve the user experience, we're allowing users to trigger the "Trigger" action directly from the Dashboard. This helps them quickly identify issues while debugging the workflow.
There was a problem hiding this comment.
Let me know what you think, i am fine removing it because now the trigger is less predicted, but in the same time it will cost workflow debugging flow.
There was a problem hiding this comment.
I tend to agree with sok, I think trigger should be predictable and throw an error when in a bad step, not the type of things you want to learn about in prod
There was a problem hiding this comment.
Following on this, @djabarovgeorge is right. Without a more sophisticated way to test an inactive workflow from the Dashboard we will need this hack in the logic to allow Novu users to test workflows from the Dashboard.
But this means we must ensure the API request comes from the Dashboard. That is, we should verify the source and the fact that a JWT (not an API key) was used for authentication.
There was a problem hiding this comment.
How do we feel about adding additional indicators for this case?
- If the status is
ERROR, we could either:- Change the button color, or
- Add a warning icon to highlight the workflow status.
- If the status is
ERROR, the payload could include an extra parameter to show the trigger override, for example:
payload:
{
"__debugModeBypassStatus": true,
...otherParams
}
the API will check the __source and this new param __debugModeBypassStatus
| return { | ||
| acknowledged: true, | ||
| status: TriggerEventStatusEnum.NOT_ACTIVE, |
There was a problem hiding this comment.
Can we report a better status here? And potentially also pass the actual errors in the response? I would also actually throw a 4XX error here instead of returning 200
There was a problem hiding this comment.
The status code depends on how we handled the trigger endpoint. Returning 4XX can be a breaking change for some customers who don't handle errors when doing novu.trigger. So, for now, we should return whatever the API was returning.
There was a problem hiding this comment.
Didn't thought of the active one, but for the Error checks, I would still return an error
What changed? Why was the change needed?
We added issue management under the workflow entity, but we never enforced its usage. This PR ensures that notifications won't be triggered for statuses that are not active.
At the same time, to improve the user experience, we're allowing users to trigger the "Trigger" action directly from the Dashboard. This helps them quickly identify issues while debugging the workflow.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer