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.

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

Send new request email notifications to power users #4462

Merged
merged 4 commits into from
Jan 14, 2022

Conversation

sephrat
Copy link
Contributor

@sephrat sephrat commented Jan 10, 2022

This provides the possibility to send "New Request" notifications to power users instead of the Ombi administrator.
On top of that, if Ombi has multiple admins, an email will be sent to all admins instead of the "Admin Email" set up in the notifications settings.

I tagged this as a draft because I'm not sure about the setting I added:

  1. I'm not sure it should even be a setting since it makes more sense toggled on IMO. Why send the notification to a fraction of the population that is allowed to approve the request?
  2. This could be extended to mobile notifications (at least), but then that would make it a more global setting than an email one - probably a "New Request" notification template one?

@tidusjar
Copy link
Collaborator

Yeah,

I mentioned a setting to prevent any issues and make it backwards compatible. But I agree it does make sense to be a default, if someone doesn't like it then we can alter that in the future.

Also regarding the other notification types, it should really apply to all (inc. mobile).

@sephrat sephrat marked this pull request as ready for review January 11, 2022 15:24
@sephrat
Copy link
Contributor Author

sephrat commented Jan 11, 2022

I've added the logic to mobile notifications, although I could not test this myself.
I could not find the logic for recipients determination for other notifications types such as Pushbullet, Discord, ... . How do you take into account users' tokens and how do you know in which case to send it to who?

@tidusjar
Copy link
Collaborator

For the other agents, the notifications are published to the channels, so it's only really Email and Mobile that will go to individual users currently

@sephrat sephrat requested a review from tidusjar January 14, 2022 11:53
@tidusjar
Copy link
Collaborator

We probably want to apply this to the following notification types:

  • NewIssue
  • IssueComment
  • AddedToRequestQueue

I think we should just remove GetAdmins() from the MobileNotification.cs and always use GetPrivilegedUsersPlayerIds

@sephrat
Copy link
Contributor Author

sephrat commented Jan 14, 2022

Mmmh, I thought about it but tend to disagree on this. My interpretation of the difference between an admin and a power user is as follows:

  • Admins are tech-savvy users, they're the ones who will know about the *arr apps, so if there's an issue with a media or an integration with the *arr, they're the ones who can sort this.
  • Power users are users who can also review requests, they're not necessarily tech-savvy. They don't have access to the settings page so they're not supposed to know how to sort out issues or integration errors.

Does that make sense?

@tidusjar
Copy link
Collaborator

Fair, you got me there. I agree with your points... I guess that they couldn't resolve issues either in that case...

@tidusjar tidusjar merged commit 10cc0c0 into Ombi-app:develop Jan 14, 2022
@sephrat sephrat deleted the new-request-notification branch January 18, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants