Draft: Email notifications #31
Conversation
services/licco.py
Outdated
| if False and status: # NOTE: this endpoint works, but we don't have approver's emails | ||
| project_name = prj["name"] | ||
| project_id = prj["_id"] | ||
| # @TODO: we need a way to access approver users email |
There was a problem hiding this comment.
I think that the user names that we pass through to the flaskauthnz authorization are also the email addresses @slac.stanford.edu I'll go back and verify this
start.py
Outdated
| app.config["NOTIFICATIONS"] = {"service_url": "http://localhost:3000"} | ||
|
|
||
| # read credentials file for notifications module | ||
| credentials_file = "./credentials.ini" |
There was a problem hiding this comment.
Note for our future selves, we probably want a place (optional argument or otherwise) to specify where this credentials file is
There was a problem hiding this comment.
I think we should look towards moving environment variables into a config file (which currently does not exist), as otherwise you lose track of what is even possible to configure and you have to dig into the source code.
notifications/notifier.py
Outdated
| self.email_sender = EmailSender(email_config) | ||
| self.service_url = service_url | ||
|
|
||
| def notify_project_approver(self, approver_emails: [], project_name: str, project_id: str): |
There was a problem hiding this comment.
Mutable default arguments here as well.
Is the intention to make a new method for each notification type we're sending? It might be worth just having a single send_notification method that we provide a type to, then gather all the notification templates in a dictionary elsewhere.
We could make the notification types an Enum, to keep the options limited
There was a problem hiding this comment.
Yes the initial idea was that each type of notification would have its own method, but that's up for a discussion.
Having a single method handling all integration points could be a bit problematic. The problem is that different notifications will need a different message format:
-
email clients expect html formatting (and sometimes even plain text) as well as an email subject and the sender's username (e.g., Slac NoReply) that will be shown in an email client
-
services like Slack expect a message in markdown, the name of the chat room and probably no subject. If notifying specific people is expected, then we need to somehow associate users with their Slack username (if they even have an account).
-
Jira and friends probably expect something completely different (e.g., an issue id number).
Therefore, if we ever expect to extend the notifier, I guess we should have one method handling each of the integration points (due to their specific differences):
notifier.send_emails(from, users, approve_user_subject, approve_user_email_msg.format(var1, var))
notifier.send_slack_msg(users, approve_user_slack_msg.format(var1, var2))There was a problem hiding this comment.
I'm on board with the one-method-per-destination. Since the only thing that changes within one destination is the message contents, it didn't really make sense to duplicate the methods to me. This seems like a good amount of de-duplication for now. (maybe there could be more, but that can be a later optimization)
…ged via environment variable
Work in progress