- Notifications
You must be signed in to change notification settings - Fork907
fix: ensure targets are propagated to inbox#16985
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Rather than give targets special treatment, we should put it in thepayload like everything else. This correctly propagates notificationtargets to the inbox table without much code change.
| @@ -74,7 +74,7 @@ func (s *StoreEnqueuer) EnqueueWithData(ctx context.Context, userID, templateID | |||
| dispatchMethod = metadata.CustomMethod.NotificationMethod | |||
| } | |||
| payload, err := s.buildPayload(metadata, labels, data) | |||
| payload, err := s.buildPayload(metadata, labels, data, targets) | |||
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 it the first time we're processing targets? Before we merge this one, I'd like to understand the use case. AFAIRtargets is an array of UUIDs, and that can be a combination of user UUID, template UUID, workspace UUID. Are we going to discover types of resources?
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.
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 see, I missed the context. Obviously it is too late for this feedback, so ignore the comment 👍
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.
LGTM 👍
ef62e62 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Currently the
targetscolumn ininbox_notificationsdoesn't get filled. This PR fixes that. Rather than give targets special treatment, we should put it in the payload like everything else. This correctly propagates notification targets to the inbox table without much code change.