Skip to content

Conversation

colinodell
Copy link
Member

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:

  • User Preferences
    • (Has the user explicitly opted into or out of this notification?)
  • Default Preferences
    • (Sane defaults optionally set by the developer per notification and/or transport type)
  • Fallback Preference
    • (A global option if nothing above returned an explicit preference)

This PR makes that possible.

Changes

Notable changes include:

  • Added a new pkg/notifier/preference package with primitives for defining and checking preferences
  • Abstracted per-user preference fetching into a new user.PreferenceProvider
  • Added a new mailroom.WithDefaultPreferences() function to provide default preferences
    • The default notifier will check the user.PreferenceProvider first, falling back to the default preferences if needed'
    • Default preferences can be provided as a fixed value (via preference.Default([bool])), a custom function, a chain of preference providers, etc.

@colinodell colinodell self-assigned this Jun 17, 2025
Copy link

codecov bot commented Jun 17, 2025

Codecov Report

Attention: Patch coverage is 91.17647% with 9 lines in your changes missing coverage. Please review.

Project coverage is 87.38%. Comparing base (9a1c394) to head (8da442e).
Report is 8 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
server.go 52.94% 7 Missing and 1 partial ⚠️
pkg/user/user.go 50.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@colinodell colinodell marked this pull request as ready for review June 17, 2025 14:03
Base automatically changed from refactor to main June 17, 2025 15:23
userStore user.Store
transports []Transport
transports []Transport
preferences preference.Preferences
Copy link
Contributor

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?

Copy link
Member Author

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:

mailroom/server.go

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that helps!

@colinodell colinodell requested a review from zhammer June 17, 2025 17:49
@colinodell colinodell merged commit 1958d67 into main Jun 17, 2025
11 checks passed
@colinodell colinodell deleted the preferences branch June 17, 2025 17:53
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.

2 participants