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.

Sign up for GitHub

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

Jump to bottom

Support user notification preferences (backend) #7349

Merged
merged 28 commits into from
Nov 15, 2024
Merged

Support user notification preferences (backend) #7349

merged 28 commits into from
Nov 15, 2024

Conversation

ryaplots
Copy link
Contributor

Summary

References #7308

Changes

  • Add new Enum for NotificationType
  • Add NotificationPreferences message
  • Add db migrations
  • Use emailReceiverIDs instead of req.Email

Testing

Steps

...

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Testing: The steps/process to test this feature are clearly explained including testing for regressions.
  • Infrastructure: If infrastructural changes (e.g., new RPC, configuration) are needed, a separate issue is created in the infrastructural repositories.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@github-actions github-actions bot added c/identity server This is related to the Identity Server compat/db This could affect Database compatibility ui/web This is related to a web interface labels Oct 17, 2024
@ryaplots ryaplots force-pushed the feature/notification-preferences branch 2 times, most recently from d995608 to 90e6b02 Compare October 28, 2024 09:34
@ryaplots ryaplots force-pushed the feature/notification-preferences branch from d4a3916 to 4fee592 Compare October 29, 2024 14:49
@ryaplots ryaplots force-pushed the feature/notification-preferences branch 3 times, most recently from 5281ab0 to 9b5c430 Compare October 30, 2024 10:39
@ryaplots ryaplots force-pushed the feature/notification-preferences branch from 9b5c430 to ed7391b Compare October 30, 2024 10:59
@ryaplots ryaplots marked this pull request as ready for review October 30, 2024 13:05
@ryaplots ryaplots requested review from a team as code owners October 30, 2024 13:05
@ryaplots ryaplots requested review from johanstokking and removed request for a team October 30, 2024 13:05
@ryaplots ryaplots added this to the v3.33.0 milestone Oct 30, 2024
johanstokking
johanstokking reviewed Oct 31, 2024
View reviewed changes
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Looks great, I think @nicholaspcr is the main reviewer.

pkg/identityserver/store/migrations/20241001000000_email_notification_preferences.down.sql Outdated Show resolved Hide resolved
api/ttn/lorawan/v3/notification_service.proto Outdated Show resolved Hide resolved
nicholaspcr
nicholaspcr reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@nicholaspcr nicholaspcr left a comment

Choose a reason for hiding this comment

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

A few more comments but it is looking good.

pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
@ryaplots ryaplots force-pushed the feature/notification-preferences branch from a4b6417 to 0c312fc Compare November 12, 2024 14:56
johanstokking
johanstokking approved these changes Nov 13, 2024
View reviewed changes
Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Code owned files (API) LGTM with a minor comment to address

api/ttn/lorawan/v3/notification_service.proto Outdated Show resolved Hide resolved
pkg/ttnpb/notification_service.go Outdated Show resolved Hide resolved
KrishnaIyer
KrishnaIyer approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@KrishnaIyer KrishnaIyer left a comment

Choose a reason for hiding this comment

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

LGTM with a few small comments.

api/ttn/lorawan/v3/user.proto Outdated Show resolved Hide resolved
pkg/identityserver/email_notification_preferences_test.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
pkg/identityserver/notification_registry.go Outdated Show resolved Hide resolved
@ryaplots ryaplots merged commit 6dcf0be into feature/user-notification-preferences Nov 15, 2024
15 checks passed
@ryaplots ryaplots deleted the feature/notification-preferences branch November 15, 2024 14:13
@ryaplots ryaplots mentioned this pull request Nov 19, 2024
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicholaspcr nicholaspcr nicholaspcr approved these changes

@johanstokking johanstokking johanstokking approved these changes

@KrishnaIyer KrishnaIyer KrishnaIyer approved these changes

@PavelJankoski PavelJankoski Awaiting requested review from PavelJankoski PavelJankoski is a code owner automatically assigned from TheThingsNetwork/stack-reviewers-3

Assignees

@ryaplots ryaplots

Labels
c/identity server This is related to the Identity Server compat/db This could affect Database compatibility ui/web This is related to a web interface
Projects
None yet
Milestone
v3.33.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ryaplots @KrishnaIyer @johanstokking @nicholaspcr