Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

renchap
Copy link
Member

@renchap renchap commented Oct 23, 2024

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

…l never be working again (invalid tokens)

See mastodon/mastodon#32626 for the PR implementing this in Mastodon
Copy link

@ThisIsMissEm ThisIsMissEm left a 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

Copy link

@timbray timbray left a 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
Copy link

@timbray timbray Nov 4, 2024

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?

Copy link

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}
Copy link

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?

Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants