Skip to content

Conversation

@ceorourke
Copy link
Member

This is one of several PRs to update the notification settings manager functions to return a mapping of providers to a list of users instead of a list of users (what we had previously) to account for the fact that email is no longer the only means of contacting a user.

Ex: {"email": [<User 1>, <User 2>], "slack", ["User 1, User 3]}

NotificationSettingTypes.ISSUE_ALERTS,
)
output = []
for provider, value in mapping.items():
Copy link
Contributor

@scefali scefali Apr 8, 2021

Choose a reason for hiding this comment

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

@ceorourke seems like you could use filter here to build output instead of doing it itereatively

@mgaeta
Copy link
Contributor

mgaeta commented Apr 8, 2021

You should delete the unused functions _get_setting_value_from_mapping, should_user_be_notified, and should_be_participating.

Copy link
Contributor

@mgaeta mgaeta 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

@ceorourke ceorourke force-pushed the ceorourke/API-1775 branch from 27e6a00 to e3cc8d5 Compare April 8, 2021 19:58
@ceorourke ceorourke merged commit 081b361 into master Apr 8, 2021
@ceorourke ceorourke deleted the ceorourke/API-1775 branch April 8, 2021 21:08
shruthilayaj pushed a commit that referenced this pull request Apr 13, 2021
* feat(notification platform): Handle multiple providers
@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants