-
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
Add support for Unsubscribe-URL
#8
base: master
Are you sure you want to change the base?
Conversation
…l never be working again (invalid tokens) See mastodon/mastodon#32626 for the PR implementing this in Mastodon
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.
My Go isn't fantastic, but I think this looks okay to me. The only question I'd have is if multiple workers may be processing the same notification URL? Then you might need to synchronise unsubscribe status somehow
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.
Don't see anything here that would offend the Go-literate eye. Couple minor comments.
isProduction bool | ||
notification *apns2.Notification | ||
unsubscribeUrl string | ||
requestLog *log.Entry // For logging with datadog context |
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.
A little gross to have support for specific commercial non-OSS products wired into the code? Would this log.Entry usable with a DataDog competitor?
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.
Logrus provides means to write custom hooks for any need you may have, so yes it would be usable.
A lot of third party hooks already exist, some of which are listed here.
messageChan <- &Message{isProduction, notification, requestLog} | ||
unsubscribeUrl := request.Header.Get("Unsubscribe-Url") | ||
|
||
messageChan <- &Message{isProduction, notification, unsubscribeUrl, requestLog} |
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.
If there's no Unsubscribe-url
header you' send "" along in unsubscribeUrl
, that OK?
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 if
statement at line 81 ensures that the behavior is unchanged in this case. The new unsubscribed
field in the entry does not cause issues elsewhere if not read.
Support
Unsubscribe-URL
header to unregister subscriptions that will never be working again (invalid tokens)See mastodon/mastodon#32626 for the PR implementing this in Mastodon