-
Notifications
You must be signed in to change notification settings - Fork 3
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
feature/MEDITOR-916-add-webhooks #67
Conversation
Adds webhooks without consuming response; future stories could act upon response and/or show status of webhooks in document history panel.
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.
Related to the BitBucket PR for mEditor models with the same tag.
@@ -32,3 +32,6 @@ LOG_LEVEL= | |||
|
|||
# used when the origin URL of mEditor's host is needed | |||
MEDITOR_ORIGIN=http://localhost | |||
|
|||
# Escaped JSON array as a string. E.g., "[{ \"token\":\"dGhpcy1pcy1hLXRva2VuLWZvci1lbmRwb2ludC0x\",\"URL\":\"http://example.com/1\" }]" |
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.
The MVP implementation puts the webhook config (token and URL) in the environment. Future improvements should probably move this into a model (token would be treated as a sensitive field).
.gitignore
Outdated
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.
We have Python dependencies, so we can let our pip
users create an env
or venv
directory.
packages/app/documents/service.ts
Outdated
] | ||
*/ | ||
|
||
log.debug(response) |
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.
This was intentional, but it's not necessary.
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.
Can we actually change _error to error and log the error here if it exists? During testing, it wasn't immediately clear what was failing with my .env config
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.
Invokes the matching webhook on createDocument
and changeDocumentState
.
@@ -59,6 +59,10 @@ export async function getModel( | |||
// execute the macro templates for this model and get their values | |||
const [error, populatedTemplates] = await runModelTemplates(model) | |||
|
|||
if (error) { |
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.
Broken model templates were not properly handling errors, so we now re-raise them.
@@ -65,6 +65,15 @@ body { | |||
margin-bottom: 2rem; | |||
} | |||
|
|||
.form-group label:has(input[type='checkbox']) { |
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.
This adjusts the spacing between the RSJF checkbox and its label. That had been bothering me for too long, haha.
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 this has always annoyed me too
@@ -87,3 +92,17 @@ export async function respondAs( | |||
}) | |||
} | |||
} | |||
|
|||
export async function parseResponse(response: Response) { |
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.
Reads the Content-Type
header and parses as indicated.
|
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 awesome, very clean implementation of this. Had a couple comments more relating to additions for helping to debug
packages/app/documents/service.ts
Outdated
] | ||
*/ | ||
|
||
log.debug(response) |
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.
Can we actually change _error to error and log the error here if it exists? During testing, it wasn't immediately clear what was failing with my .env config
packages/app/documents/service.ts
Outdated
] | ||
*/ | ||
|
||
log.debug(response) |
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.
Same comment here as the one in createDocument
@@ -119,8 +120,28 @@ async function listUniqueFieldValues( | |||
} | |||
} | |||
|
|||
async function getAllWebhookURLs(): Promise<ErrorData<string[]>> { |
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.
Nice! This ended up being a really simple macro!
@@ -65,6 +65,15 @@ body { | |||
margin-bottom: 2rem; | |||
} | |||
|
|||
.form-group label:has(input[type='checkbox']) { |
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 this has always annoyed me too
packages/app/webhooks/service.ts
Outdated
}) | ||
|
||
if (!response.ok) { | ||
throw new HttpException(ErrorCode.BadRequest, response.statusText) |
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.
This error is what I was referencing in a previous comment, about logging what failed during the invocation.
It would be nice to see the actual error, 404, 400, etc coming from the webhook endpoint (windmill) if possible
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.
Expanded HttpException to accept either a status code or an error code (ce7d201) so that we can throw the API's actual error. Added some tests as documentation.
Adds MVP webhooks to mEditor. Does not handle webhook response, have a callback, or poll the endpoint for async tasks. Future improvements would probably add webhook status to document history.