Implement a direct notify API endpoint#12529
Conversation
This was requested by IET and @euanmillar as in many integration platforms like DHIS2, OpenFn etc. making a sequential request for create + notify is significantly more complex than making a singular request
|
Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:
|
|
ℹ️ Coverage metrics explained: |
📊 commons test coverage |
📊 events test coverage |
| const eventConfigs = await getInMemoryEventConfigurations(token) | ||
| const tokenScopes = getScopes(token) | ||
|
|
||
| const eventConfig = eventConfigs.find((c) => c.id === eventType) | ||
| if (!eventConfig) { | ||
| throw new TRPCError({ | ||
| code: 'BAD_REQUEST', | ||
| message: `No configuration found for event type: ${eventType}` | ||
| }) | ||
| } | ||
|
|
||
| // Must have record.create scope for this event type | ||
| if (!canUserCreateEvent(tokenScopes, eventType)) { | ||
| throw new TRPCError({ code: 'FORBIDDEN' }) | ||
| } |
There was a problem hiding this comment.
We could write a separate userCanCreateAndNotifyEvent middleware that does the same getRawInput() like userCanCreateEvent pattern but parses with CreateAndNotifyInput and checks both scopes. That would clean up the handler nicely.
There was a problem hiding this comment.
Would that be just another layer of indirection though? I have some bad experiences with middlewares generally speaking 😬 Never really understood what the benefit is in comparison to doing something like
await userCanCreateAndNotifyEvent(user, event)
as the first step of the handler 🤔
There was a problem hiding this comment.
A plain function call might be the better fit for this case. The suggestion I made about extracting a middleware was based on the existing pattern, but the existing pattern itself is a workaround. Given the handler already has parsed input, a direct function call could be cleaner. Should be easier to debug and easier to mainitain
| config | ||
| }) | ||
|
|
||
| // Step 2: Idempotency guard. |
There was a problem hiding this comment.
Idempotency for create event is handled in createEvent 👍
There was a problem hiding this comment.
@Nil20 wouldn't that only ensure if the event exists or not?
There was a problem hiding this comment.
Yeah! No need to check for CREATE's idempotency. Just checking for NOTIFY action's idempotency is the correct behavior
| declaration: declaration, | ||
| annotation, | ||
| createdAtLocation, | ||
| keepAssignment |
There was a problem hiding this comment.
keepAssignment should be necessary? We aren't doing actions like declare+register, so keeping assignment shouldn't probably be here
|
Maybe we should switch target branch to |
3 participants
This was requested by IET and @euanmillar as in many integration platforms like DHIS2, OpenFn etc. making a sequential request for create + notify is significantly more complex than making a singular request