-
Notifications
You must be signed in to change notification settings - Fork 90
feat(backend): also publish webhooks to operators if primary recipient is tenant #3368
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
Conversation
7cfa385
to
14fe53f
Compare
a5ce199
to
98b1291
Compare
4260036
to
5c51d6d
Compare
98b1291
to
64973e7
Compare
1950deb
to
d241f0f
Compare
886bdac
to
38510de
Compare
🚀 Performance Test ResultsPerformance test summary not found. 📜 Logs
|
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.
Looks good, some minor comments.
Also, I think it would be good to enable this feature under an environment variable eg (SEND_ALL_WEBHOOKS_TO_OPERATOR
). Can do it separately after this is merged in
Also some tests are failing
@@ -50,6 +51,7 @@ export const listWebhooks = async ( | |||
`, | |||
variables: args | |||
}) | |||
console.log('response.data.webhookEvents=', response.data.webhookEvents) |
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.
console.log('response.data.webhookEvents=', response.data.webhookEvents) |
@@ -32,6 +33,7 @@ interface GetPageOptions { | |||
|
|||
export interface WebhookService { | |||
getEvent(id: string): Promise<WebhookEvent | undefined> | |||
getWebhook(id: string): Promise<Webhook | undefined> |
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.
is getWebhook
used?
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.
Looks like its used for testing
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 managed to rewrite the tests such that it's not necessary anymore. So it can be removed.
@@ -379,12 +379,13 @@ async function getOrPollByUrl( | |||
const existingWalletAddress = await getWalletAddressByUrl(deps, url) | |||
if (existingWalletAddress) return existingWalletAddress | |||
|
|||
await WalletAddressEvent.query(deps.knex).insert({ | |||
await WalletAddressEvent.query(deps.knex).insertGraph({ |
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.
Maybe we can make an nice-to-have issue for this:
We should check the path/prefix of the wallet address, and if it matches a particular tenant, we send that to them instead of the operator tenant
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.
Captured in #3414
38510de
to
3324c3b
Compare
) | ||
const formattedSettings = formatSettings(settings) | ||
|
||
await sendWebhookEvent(deps, webhook, formattedSettings) |
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.
do we need to pass operatorSettings
into sendWebhookEvent
? Otherwise i dont think we ever do the duplicate publish to operator?
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.
So sendWebhookEvent
does not actually distinguish between if a webhook is for an operator or a tenant. Rather, webhooks for tenants and the operator are all occupying the same queue that sendWebhookEvent
processes. So each time another service determines that a webhook event has occurred, it will create 1-2 webhooks depending on if the recipient is the operator or not.
This does conflict with the concept I'm introducing in this PR which is the distinction between webhooks and webhook events. This would probably make more sense if it were renamed to processWebhook
instead of processWebhookEvent
.
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.
as discussed in slack, it looks like the optional operatorSettings
just needs to be removed from sendWebhookEvent
args then
Changes proposed in this pull request
Context
Fixes #3283.
Checklist
fixes #number
user-docs
label (if necessary)