-
Notifications
You must be signed in to change notification settings - Fork 18
Refactor preferences to allow custom defaults and chaining #83
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
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #83 +/- ##
==========================================
+ Coverage 87.24% 87.38% +0.13%
==========================================
Files 18 20 +2
Lines 894 951 +57
==========================================
+ Hits 780 831 +51
- Misses 98 105 +7
+ Partials 16 15 -1 ☔ View full report in Codecov by Sentry. |
pkg/notifier/default.go
Outdated
userStore user.Store | ||
transports []Transport | ||
transports []Transport | ||
preferences preference.Preferences |
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.
this is preferences for an individual user?
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.
No, it's the per-user lookup function with a default fallback:
Lines 51 to 54 in 8da442e
s.notifier = notifier.New(s.transports, preference.Chain{ | |
user.NewPreferenceProvider(s.userStore), | |
s.defaultPreferences, | |
}) |
preference.Preference
probably isn't the best name for that interface, so I've just renamed it to preference.Provider
via commit 8da442e.
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.
that helps!
This PR shifts responsibility for delivery preferences out of the user objects and into a customizeable, chainable system.
Context
Previously, we assumed users should always receive notifications by default unless they explicitly opted-out. This makes sense in most cases, but there may be certain noisy notifications that should instead be opt-in. Ideally we'd have a way for Mailroom to determine whether to deliver based on checking the following things in the following order:
This PR makes that possible.
Changes
Notable changes include:
pkg/notifier/preference
package with primitives for defining and checking preferencesuser.PreferenceProvider
mailroom.WithDefaultPreferences()
function to provide default preferencesuser.PreferenceProvider
first, falling back to the default preferences if needed'preference.Default([bool])
), a custom function, a chain of preference providers, etc.