- Notifications
You must be signed in to change notification settings - Fork928
feat: add notification preferences business logic & APIs#14117
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
feat: add notification preferences business logic & APIs#14117
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dannykopping commentedAug 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@dannykopping and the rest of your teammates on |
alwaysmeticulousbot commentedAug 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Meticulous was unable to execute a test run for this PR because the most recent commit is associated with multiple PRs. To execute a test run, please try pushing up a new commit that is only associated with this PR. Last updated for commit11eacb8. This comment will update as new commits are pushed. |
4a0e0f2
to8041cc7
Compare6e027ef
to01e7b50
Compare30f3525
to62f6dec
Compare01e7b50
to0aef037
Compare88b9796
toce7f0c3
Comparefe8aa2d
to3516354
Compare0386dab
to8bec5b1
Compare8bec5b1
tof2926a3
Compare3516354
to2dd0b09
Compareb3679fe
tod627fef
CompareSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Test fixSigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
2dd0b09
to7bb1ee0
Compareinput := database.UpdateUserNotificationPreferencesParams{ | ||
UserID: user.ID, | ||
NotificationTemplateIds: make([]uuid.UUID, 0, len(prefs.TemplateDisabledMap)), | ||
Disableds: make([]bool, 0, len(prefs.TemplateDisabledMap)), |
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.
Suggest settingEnabled
in the external API instead.
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.
Yeah I vacillated on this quite a bit.
I think this is more correct though because the feature is to disable notifications, and this happens to line up with the database design as well.
Uh oh!
There was an error while loading.Please reload this page.
33e9bc9
intodk/notification-prefs/admin-template-methodUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Part ofcoder/internal#19
Users should be empowered to control which notifications they do not wish to receive.
Added two new routes:
GET /users/{users}/notifications/preferences
PUT /users/{users}/notifications/preferences
When a user has disabled a notification template in their preferences, we need to ensure that:
notifications.ErrCannotEnqueueDisabledNotification
errorThe database logic was implemented in#14100, see the
inhibit_enqueue_if_disabled
trigger.