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(growth): pull out helper methods for organization_invite_request_details #29823

Merged
merged 4 commits into from
Nov 8, 2021

Conversation

scefali
Copy link
Contributor

@scefali scefali commented Nov 5, 2021

This PR pulls out some helper methods from OrganizationInviteRequestDetailsEndpoint such that we can leverage common code with an upcoming PR 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.

@scefali scefali requested a review from a team as a code owner November 5, 2021 18:51
@scefali scefali requested review from mgaeta, a team and wedamija November 5, 2021 18:51
Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

The refactor looks good, thanks for doing this. All my comments are just nits, lgtm

I agree that we probably need somewhere other than utils for this kind of thing, but we can figure this out later.

Probably the OrganizationMember class is a good idea, like you suggested. Another option might be the manager, since these methods don't operate on a specific instance of an OrganizationMember. A third option is making something like an invitations module/django app and placing all related code there, maybe a bit overkill though.

Comment on lines +49 to +50
return OrganizationMember.objects.get_member_invite_query(member_id).get(
organization=organization
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense at all for organization to be passed as a parameter to get_member_invite_query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wedamija I purposely left it out because I have a query where I don't have the organization. See: https://github.com/getsentry/sentry/pull/29708/files#diff-38bebb13f20409b4ea44e977a5c976b696c2e8f3e1ec02855b2581e5d422f4e5R378-R384

The alternative was to add the organization_id to the callback_id but I figured it was better to have a more minimalist approach for what data I need to send.

@@ -0,0 +1,92 @@
from django.conf import settings
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It might be a good idea to add tests for this file, now that this functionality is stand-alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wedamija good call, I can add some basic tests

organization.get_option("sentry:join_requests") is False
and member.invite_status == InviteStatus.REQUESTED_TO_JOIN.value
):
raise serializers.ValidationError(ERR_JOIN_REQUESTS_DISABLED)
Copy link
Member

Choose a reason for hiding this comment

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

Will these functions be used outside of APIs? If that's the case, we might want to raise generic errors here that can be intercepted by the api and other places that are using this function.

Just something simple like:

class CannotInvite(Exception):
    # Probably needs a better name
    pass

that you can throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wedamija are you saying to raise a specific error then have the serializer catch that error and raise a ValidationError? I think that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah exactly, it just makes these functions a little more generic and non-api specific

@scefali
Copy link
Contributor Author

scefali commented Nov 5, 2021

The refactor looks good, thanks for doing this. All my comments are just nits, lgtm

I agree that we probably need somewhere other than utils for this kind of thing, but we can figure this out later.

Probably the OrganizationMember class is a good idea, like you suggested. Another option might be the manager, since these methods don't operate on a specific instance of an OrganizationMember. A third option is making something like an invitations module/django app and placing all related code there, maybe a bit overkill though.

Well, all those helper methods take an instance of the OrganizationMember as the first argument so we can just do it on that class, i'll make that change

@scefali scefali merged commit b7f98bb into master Nov 8, 2021
@scefali scefali deleted the ref/pull-out-member-helpers branch November 8, 2021 16:30
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2021
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.

3 participants