ref(notifications): org request notification #29362
Conversation
| self.organization = organization | ||
|
|
||
| @property | ||
| def SlackMessageBuilderClass(self) -> Type["SlackNotificationsMessageBuilder"]: |
There was a problem hiding this comment.
Hacking around an import problem. Maybe there is a better way we can have a specific Slack message builder be associated with a particular notification class.
| ) -> None: | ||
| super().__init__(notification, context, recipient) | ||
| # TODO: use generics here to do this | ||
| self.notification: ProjectNotification = notification |
There was a problem hiding this comment.
Maybe there is a better way to do this but I'm not sure
| class MessageAction: | ||
| label: str | ||
| url: str | ||
| style: Optional[str] |
There was a problem hiding this comment.
Can you make this style string an enum?
There was a problem hiding this comment.
@mgaeta I did it as a literal, I think that works too, yes?
| extra.update({"activity": self.activity}) | ||
| return extra | ||
|
|
||
| def get_headers(self) -> Mapping[str, Any]: |
There was a problem hiding this comment.
These headers are implementation details of the sentry.mail module and should be moved there. You can make X-Sentry-Project optional.
There was a problem hiding this comment.
@mgaeta So you're saying remove the get_headers method from the class and just have everything handled in the existing get_headers method as already exists? What does the X-SMTPAPI header do?
There was a problem hiding this comment.
Yes use get_headers() from sentry.mail.notifications.py. I'm not sure what the headers are for, they're from the legacy MailAdapter.
There was a problem hiding this comment.
@mgaeta Ok, we haven't been using any headers in the past for growth email notifications and since we don't know what the X-SMTPAPI does, I don't see why we'd need to add it now. How then should my notification class specify that it doesn't need headers if it can't subclass a get_headers function?
There was a problem hiding this comment.
https://docs.sendgrid.com/for-developers/sending-email/getting-started-smtp
You should just use the X-SMTPAPI header with the category parameter.
There was a problem hiding this comment.
ah, that makes sense. thanks!
There was a problem hiding this comment.
@mgaeta switched back to using get_headers in sentry.mail.notifications
src/sentry/mail/notifications.py
Outdated
| } | ||
| def build_subject_prefix(project: "Project") -> str: | ||
| key = "mail:subject_prefix" | ||
| return str( |
There was a problem hiding this comment.
In order to get build_subject_prefix() and generate_signed_link() to work with types you should avoid casting. Our best alternative right now is to explicitly annotate like:
# Explicitly typing to satisfy mypy.
subject_prefix: str = ProjectOption.objects.get_value(project, key) or options.get("mail.subject-prefix")
return subject_prefixAlso adding this file to mypy.ini will automatically send the PR to the python-typing group. They'll probably require that you move typing to a separate PR.
There was a problem hiding this comment.
@mgaeta got it, I'll remove it from mypy.ini. I'll leave the function alone aside from removing the mail_option_key argument since it's never being applied.
| for provider, recipients in participants_by_provider.items(): | ||
| notify(provider, self, recipients, self.get_context()) | ||
|
|
||
| def get_member(self, user: "User") -> "OrganizationMember": |
There was a problem hiding this comment.
I think it would make more sense to populate the member_by_user_id with a single query in case there are N admins for an organization.
recipients_cache: Optional[Set[Union["Team", "User"]]] = None
member_by_user_id: MutableMapping[int, OrganizationMember] = {}
def determine_recipients(self) -> Iterable[Union["Team", "User"]]:
if recipients_cache:
return recipients_cache
recipients_cache = ...
users = {recipient for recipient in recipients if isinstance(recipients, User)}
organization_memberships = OrganizationMember.objects.get(user__in=users, organization=self.organization)
self.member_by_user_id = {
organization_membership.user_id: organization_membership
for organization_membership in organization_memberships
}
return recipients_cache
def get_member(self, user: "User") -> "OrganizationMember":
return member_by_user_id.get(user.id)There was a problem hiding this comment.
@mgaeta I can pre-populate the member cache in determine_recipients but I still might want to fall back to a query if my member isn't in the cache, though that probably shouldn't happen.
There was a problem hiding this comment.
Yeah also maybe after we read NotificationSettings there are no members at all and we could save the query altogether.
src/sentry/mail/notifications.py
Outdated
| f"sentry-account-email-unsubscribe-{key}", | ||
| referrer, | ||
| kwargs={f"{key}_id": resource_id}, | ||
| return str( |
There was a problem hiding this comment.
This got lost in a previous comment but you should avoid casting to satisfy mypy.
signed_link: str = generate_signed_link(
user_id,
f"sentry-account-email-unsubscribe-{key}",
referrer,
kwargs={f"{key}_id": resource_id},
)
return signed_linkThere was a problem hiding this comment.
@mgaeta good catch! Let me add that in
This PR creates a
OrganizationRequestNotificationclass which is purposely built to handle various organization level requests (request to install integration, request to invite member, request to upgrade, etc). In making this more OOO friendly, I have added a class property calledSlackMessageBuilderClasswhich defines which Slack builder a notification should be used. I moved some logic to the notification class to make sub-classing easier.No actual notification classes are using
OrganizationRequestNotificationyet. That will come in future PRs.Note that this PR is just a starting place. We should do more refactoring as a follow-up to make it cleaner and less hacky.