-
Notifications
You must be signed in to change notification settings - Fork 22
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
event notifications #2108
event notifications #2108
Conversation
to get support for PUT bodies
24f7aca
to
ae6c6b6
Compare
since they're now public via the ApplicationRaw schema
at most use sites
when no notification targets are set
src/clj/rems/event_notification.clj
Outdated
(def ^:private default-timeout 60) | ||
|
||
(defn- notify! [target body] | ||
(log/info "Sending event notification to" (:url target)) |
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 wonder if it would make sense to log a bit more info here about the event itself too, like id.
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.
hmm it looks like we don't pass the event id out of the db layer. I'll log the time, type, application-id.
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.
clarification: these events don't have ids because they're directly from the command handler, not fetched from the db
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 think it'd make sense for the notified party to know the event id. It makes idempotency a bit easier.
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 now. We can continue the discussions elsewhere, in followup PRs perhaps.
for #2095
Definition of Done / Review checklist
Reviewability
API
Documentation
Testing
Follow-up