Webhook get and resend APIs #2021
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
mjh1
force-pushed
the
mh/webhook-resend
branch
from
0aebcf9 to
7d2c05a
Compare
|
Annoyingly this PR looks bigger than it really is, I had to move some code around so lots of deletes and additions of the same lines. |
|
Just realised I need to add the new API paths to api-schema.yaml |
If it'd be easy to separate the moving around on a separate commit, it'd be easier to review! no problem if it's not trivial tho |
I'll try! 👀 |
mjh1
force-pushed
the
mh/webhook-resend
branch
from
7d2c05a to
658336d
Compare
mjh1
force-pushed
the
mh/webhook-resend
branch
from
658336d to
5fccbb3
Compare
Done 👍 the first commit is the one with the real changes, the next two are just moving code around |
|
@victorges ready for review now |
| @@ -287,9 +295,45 @@ const requestsFieldsMap: FieldsMap = { | |||
| statusCode: `webhook_response.data->'response'->>'status'`, | |||
| }; | |||
|
|
|||
| app.put("/:id/log/:requestId/resend", authorizer({}), async (req, res) => { | |||
There was a problem hiding this comment.
Should this be a POST instead? Unless we somehow keep the same requestId when we resend, but I guess we don't 🤔
There was a problem hiding this comment.
So you're suggesting POST /:id/log/resend with the requestID in the request body?
There was a problem hiding this comment.
No, I think POST /:id/log/:requestId/resend is fine! It's just that a PUT .../:requestId would seem like it's updating the same request/log, instead of creating a new one. Also I think the general pattern for "actions" (in this case "resend") is to be a POST as well
| const webhook = await db.webhook.get(req.params.id); | ||
| const webhookResponse = await db.webhookResponse.get(req.params.requestId); |
There was a problem hiding this comment.
Something funny here is that the id and requestId params are redundant right? I wonder if we'll ever need an API which is just /webhook/log/:requestId instead, but let's keep it like this for now I think it's consistent with standards
There was a problem hiding this comment.
Yeah I wasn't too sure which way was preferable
| $ref: "#/components/schemas/error" | ||
| "/webhook/{id}/log/{logId}/resend": | ||
| put: | ||
| summary: Resend a webhook |
There was a problem hiding this comment.
It might be worth some further description: about what this API does
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@victorges would you mind just sanity checking the last two commits? thanks! |
packages/api/src/webhooks/cannon.ts
Outdated
| sharedSecret: sharedSecret, | ||
| }; | ||
| await db.webhookResponse.create(webhookResponse); | ||
| return webhookResponse; | ||
| } catch (e) { |
There was a problem hiding this comment.
Perhaps we should rethrow this error now, otherwise the API will respond with a 200 and an "undefined" body
There was a problem hiding this comment.
I moved the catch into the main firehook function where it's still needed. hopefully that looks good to you 9490476?w=1
What does this pull request do? Explain your changes. (required)
Implements get and resend APIs for webhook logs. So now you can retrieve info for a single webhook log and trigger a resend.
Specific updates (required)
storeResponsefunction outside of the cannon object so that it could be called by the newly writtenresendWebhookfunction.How did you test each of these updates (required)
Tested locally and wrote a unit test.
Checklist