Support user notification preferences

KrishnaIyer commented 2 months ago

Summary

Support user notification preferences. Replaces https://github.com/TheThingsIndustries/lorawan-stack/issues/3280

Current Situation

Currently users receive emails for all notification types except collaborator_changed. We want to allow users to choose what kind of emails they receive. All notifications are displayed in the console. So some users prefer to not receive emails

Why do we need this? Who uses it, and when?

We want to allow users to choose what kind of emails they receive.

Proposed Implementation

Backend

[x] Introduce a new Enum for NotificationType in the API. Currently we use hardcoded strings like user_requested or collaborator_changed in pkg/email/templates and when we use the CreateNotificationRequest message.
- [x] We need to do a replacement in all the places where this is used and also test that email templates are not affected by this.
[x] Introduce a NotificationPreferences message inside the User message.
- [x] This contains a single message repeated NotificationType email_notification_types. This is a list of NotificationType for which we would send emails.
- [x] This can be updated by the User based to select which notifications they want emails for. We need some validation to filter out duplicates.
- [ ] There are special admin emails user_requested and client_requested, both of which require admin actions. These emails are very infrequent and should not be skipped. So the API should either ignore these enums or return a validation error.
[x] Add DB entries and migrations.
[x] Change the lookupNotificationReceivers function to return a second emailReceiverIDs argument. This is a filtered list of receiver IDs.
- [x] A receiver is placed in this list if the email notification type in req is allowed for the user in their preferences.
[x] In CreateNotification, call is.SendNotificationEmailToUserIDs only emailReceiverIDs is not empty.
[x] Deprecate the email field of the Notification message. We should instead get this from the user preferences
- [ ] Lots of unit and production testing

The backend needs to target the v3.33 branch since this requires a database migration.

UX

[ ] Edit the /console/user-settings/profile view and rename it to Account or Preferences, whichever makes sense.
[ ] Add a new "Email Notifications" section.
- [ ] This contains a list of checkboxes, similar to choosing Rights on API keys. Example below
- [ ] The options are the NotificationType enums.
- [ ] We also somehow have an option to choose All or None.
- [ ] Let's document here that emails that require admin actions such as approving users or OAuth clients cannot be skipped.
[ ] Plug this page into the API above and test.

Contributing

[X] I can help by doing more research.
[ ] I can help by implementing the feature after the proposal above is approved.
[X] I can help by testing the feature before it's released.

Validation

[ ] The feature is tested in a staging environment.
[ ] The feature is documented in The Things Stack Documentation

Code of Conduct

[X] I agree to follow TTN's Community Code of Conduct.

KrishnaIyer commented 2 months ago

@nicholaspcr: Please let me know if the backend part makes sense?

@pierrephz: Can you think of a draft wireframes for the UX? Please ping me if you have doubts.

nicholaspcr commented 2 months ago

@nicholaspcr: Please let me know if the backend part makes sense?

Looks pretty good to me but I do have a question, would it be more efficient to have a NotificationsFilters instead of NotificationsPreferences? That way what we are storing on the user are only the notification types which have to be filtered.

Additionally I believe its easier for users to set what they don't want rather than the opposite.

KrishnaIyer commented 2 months ago

Looks pretty good to me but I do have a question, would it be more efficient to have a NotificationsFilters instead of NotificationsPreferences? That way what we are storing on the user are only the notification types which have to be filtered.

The question here is the difference of what the default behavior is, a. Do users receive all emails by default and they choose what not to receive? (OR) b. Do users receive no emails by default and they can enable it for certain ones?

User experience-wise, (b) is better since all notifications are available in the Console by default.

ryaplots commented 1 month ago

@KrishnaIyer @nicholaspcr @pierrephz Considering that user_requested and client_requested require admin actions, they should be selected by default and the de-selection should be disabled in the UI. There should also be a message there saying that these require user action and that they are infrequent, so they cannot be opted out of. What do you think? For the backend, I am not sure if it's best to ignore these or return an error. I would say error. In case one of these gets through, then it's nice to have some kind of feedback. Have them as "not allowed" options.

KrishnaIyer commented 1 month ago

Considering that user_requested and client_requested require admin actions, they should be selected by default and the de-selection should be disabled in the UI.

I would say let's show it in a non-editable state in the console for admins only. For the API, I think we should just ignore this if it's set in the filters.

ryaplots commented 1 month ago

@KrishnaIyer In the notifyAdminsInternal() method, can we just ignore the req.Email field that will be deprecated and the allowed notification types as well? This method is only used for user_requested and client_requested types and these notifications should always send emails to admins, so we don't need to check if the user allows the notification types.

KrishnaIyer commented 1 month ago

@KrishnaIyer In the notifyAdminsInternal() method, can we just ignore the req.Email field that will be deprecated and the allowed notification types as well? This method is only used for user_requested and client_requested types and these notifications should always send emails to admins, so we don't need to check if the user allows the notification types.

Yes I think that makes sense.

pierrephz commented 1 month ago

Email notification wireframe:

Access the Email notification settings in the side bar under User settings
Find the list of preferences (For admin, some are disabled if they require their actions)
After editing, save or discard and receive a feedback from the system

We can still discuss the list of preferences, for example, I am not sure that Temporary password should be an option there as you will always receive an email for that.

ryaplots commented 2 weeks ago

I am not sure the wireframe is correct. Do we need options for notifications like validate or login token? Also the descriptions don't seem correct as an API key can be changed for different entities and not only apps. @KrishnaIyer can you double check?

I have opened a draft PR with fixed descriptions and included only the necessary notifications - https://github.com/TheThingsNetwork/lorawan-stack/pull/7363

cc: @KrishnaIyer @pierrephz let me know if it's good

KrishnaIyer commented 2 weeks ago

@ryaplots: These are just examples. You can adapt it to the list of options that we actually provide. If you need help in drafting text for each of the options, let me know.

TheThingsNetwork / lorawan-stack