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

feat(notifications): member invite slack requests #29708

Merged
merged 24 commits into from
Nov 9, 2021

Conversation

scefali
Copy link
Contributor

@scefali scefali commented Nov 1, 2021

This PR adds Slack notifications to the member invitation and join requests hidden behind the organizations:slack-requests feature flag:

Screen Shot 2021-11-01 at 3 00 56 PM

Once a user accepts or rejects the request, we update the message:

Screen Shot 2021-11-01 at 3 01 11 PM

Copy link
Member

@ceorourke ceorourke left a comment

Choose a reason for hiding this comment

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

Overall LGTM, mostly just some classic Steve typos I only bothered mentioning cause you need to fix the missing task anyway.

src/sentry/integrations/slack/endpoints/action.py Outdated Show resolved Hide resolved

@responses.activate
@with_feature("organizations:slack-requests")
def test_request_to_join_slack(self):
Copy link
Member

Choose a reason for hiding this comment

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

For the email one I notice you check that it sends 2 (one to the manager and one to the owner), is it expected to notify both on Slack as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ceorourke yep, but I'm not checking that spec here since it's the same

@@ -1,72 +0,0 @@
from django.urls import reverse
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to remove sentry.tasks.members from server.py too cause this is making stuff angery

scefali added a commit that referenced this pull request Nov 8, 2021
…_details (#29823)

This PR pulls out some helper methods from `OrganizationInviteRequestDetailsEndpoint` such that we can leverage common code with an [upcoming PR](#29708) that allows approving member requests with Slack. This is necessary so we can avoid calling the `OrganizationInviteRequestDetailsEndpoint` endpoint internally from the Slack webhook route while reducing the amount of duplicated code.

I had to factor out an audit log method that does not require a request so that we can call it from a context that doesn't have a request coming from a logged-in user (Slack webhook).

Note I made a utility file called `src/sentry/utils/members.py` but it might make more sense to move those methods to the `OrganizationMember` class.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants