-
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): Billing alerts on usage emails #6883
Conversation
@novu/client
@novu/framework
@novu/headless
@novu/js
@novu/nest
@novu/nextjs
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/react
@novu/react-native
@novu/shared
@novu/stateless
commit: |
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
LaunchDarkly flag references🔍 1 flag added or modified
|
# where notification entities (eg. workflows, topics) are defined. | ||
# Required. | ||
bridge-url: https://api.novu-staging.co/v1/bridge/novu | ||
|
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.
😍
libs/notifications/src/workflows/usage-limits/usage-limits.workflow.ts
Outdated
Show resolved
Hide resolved
@@ -43,7 +44,7 @@ export class NovuModule extends NovuBaseModule { | |||
static register(options: typeof OPTIONS_TYPE, customProviders?: Provider[]) { | |||
const superModule = super.register(options); | |||
|
|||
superModule.controllers = [NovuController]; | |||
superModule.controllers = [applyDecorators(NovuController, options.controllerDecorators || [])]; | |||
superModule.providers?.push(registerApiPath, NovuClient, NovuHandler, ...(customProviders || [])); | |||
superModule.exports = [NovuClient, NovuHandler]; | |||
|
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: I can't add a comment on it because there's no diff, but we also need to support the injected controllerDecorators
on the registerAsync
method too to ensure consistency on both APIs.
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.
Good point! I couldn't follow the options logic or where they are defined. Should it come just from the useFactory?
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 had some trouble here too when I implemented the NestJS Framework module. I think it will require something like calling the super.registerAsync
method and accessing the defined options on the super class, but I'm not certain.
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 will check this on my local soon.
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.
Check out the NestJS HttpModule
, this covers the same pattern we need (StackOverflow ref). They've implemented their registerAsync
a little more flexibly without using the NestJS built-in ConfigurableModuleBuilder
, which grants access to async options more easily.
This is the approach we need to take for the customProviders
and controllerDecorators
properties. We can then deprecate the second method parameter which supplies only customProviders
in favour of a single configuration object for both register
+ registerAsync
.
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.
Couldn't make it work :| I'm going to merge this PR as I need to test the Stripe behaviour on staging, and will raise a separate PR for the registerAsync behaviour later today.
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.
Ok sounds good, I'll publish Framework from 1 commit this branch's merge commit to next
to avoid releasing this change in that case. We should publish the feature in totality alongside the docs.
What changed? Why was the change needed?
Screenshots
https://github.com/novuhq/packages-enterprise/pull/239
Expand for optional sections
Related enterprise PR
Special notes for your reviewer