Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

dannykopping
Copy link
Contributor

@dannykoppingdannykopping commentedAug 2, 2024
edited
Loading

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
    • Retrieves notification preferences for a given user
  • PUT /users/{users}/notifications/preferences
    • Updates notification preferences for a given user

When a user has disabled a notification template in their preferences, we need to ensure that:

  1. Messages currently in the queue which use that (now disabled) notification template are inhibited from sending
  2. Any new messages which are enqueued after the preference is set will fail when being inserted into the database, and the enqueuer will return anotifications.ErrCannotEnqueueDisabledNotification error

The database logic was implemented in#14100, see theinhibit_enqueue_if_disabled trigger.

@dannykoppingGraphite App
Copy link
ContributorAuthor

dannykopping commentedAug 2, 2024
edited
Loading

@alwaysmeticulousalwaysmeticulous
Copy link

alwaysmeticulousbot commentedAug 2, 2024
edited
Loading

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.

@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/admin-template-method branch 2 times, most recently from4a0e0f2 to8041cc7CompareAugust 2, 2024 14:40
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/user-prefs branch from6e027ef to01e7b50CompareAugust 2, 2024 14:40
@dannykoppingdannykopping changed the titleTest database business logicfeat: add notification preferences business logic & APIsAug 2, 2024
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/admin-template-method branch from30f3525 to62f6decCompareAugust 5, 2024 08:08
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/user-prefs branch from01e7b50 to0aef037CompareAugust 5, 2024 08:09
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/admin-template-method branch from88b9796 toce7f0c3CompareAugust 5, 2024 09:29
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/user-prefs branch 2 times, most recently fromfe8aa2d to3516354CompareAugust 5, 2024 09:34
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/admin-template-method branch from0386dab to8bec5b1CompareAugust 5, 2024 09:35
@dannykoppingdannykopping marked this pull request as ready for reviewAugust 5, 2024 09:54
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/admin-template-method branch from8bec5b1 tof2926a3CompareAugust 5, 2024 11:45
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/user-prefs branch from3516354 to2dd0b09CompareAugust 5, 2024 11:49
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/admin-template-method branch 2 times, most recently fromb3679fe tod627fefCompareAugust 5, 2024 11:54
Signed-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>
@dannykoppingdannykoppingforce-pushed thedk/notification-prefs/user-prefs branch from2dd0b09 to7bb1ee0CompareAugust 5, 2024 11:55
input := database.UpdateUserNotificationPreferencesParams{
UserID: user.ID,
NotificationTemplateIds: make([]uuid.UUID, 0, len(prefs.TemplateDisabledMap)),
Disableds: make([]bool, 0, len(prefs.TemplateDisabledMap)),
Copy link
Member

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.

Copy link
ContributorAuthor

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.

@dannykoppingdannykopping merged commit33e9bc9 intodk/notification-prefs/admin-template-methodAug 5, 2024
31 checks passed
@dannykoppingdannykopping deleted the dk/notification-prefs/user-prefs branchAugust 5, 2024 13:16
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsAug 5, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@mafredrimafredriAwaiting requested review from mafredri

Assignees

@dannykoppingdannykopping

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@dannykopping@johnstcn

[8]ページ先頭

©2009-2025 Movatter.jp