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(org member invite): create OrganizationMemberInviteSerializer #87475

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mifu67
Copy link
Contributor

@mifu67 mifu67 commented Mar 20, 2025

Build out the OrganizationMemberInviteSerializer, to be used by the new member invite endpoints.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 20, 2025
"member-limit:restricted": member_invite.member_limit_restricted,
"partnership:restricted": member_invite.partnership_restricted,
},
"teams": {},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into the code: we always assign role=None for invited members. Seems a little redundant to have a dict where "role" is always None—should we make this field default to a list instead?

Copy link
Member

Choose a reason for hiding this comment

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

the teams and team roles can be assigned via the UI, it could be that we're not testing it explicitly. we'll need to decide how this data will be stored here, in the existing system we appear to make the objects (OrganizationMemberTeam) but we won't be able to do that here

we could store the role and team as a mapping of team_id: role perhaps (i would suggest id instead of slug because slug is mutable)


def test_simple(self):
member_invite = self.create_member_invite(organization=self.org, email=self.email)
result = serialize(member_invite, None, OrganizationMemberInviteSerializer())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I need to pass the serializer here ;_;

Copy link
Member

Choose a reason for hiding this comment

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

hmm i was able to get it to pass by omitting the serializer

@@ -50,6 +52,32 @@ def generate_token():
return secrets.token_hex(nbytes=32)


# Causes a circular import error when importing
# from sentry.api.serializers.models.organization_member.response
_OrganizationMemberFlags = TypedDict(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea on how to fix this? :thunk:

Copy link
Member

Choose a reason for hiding this comment

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

do we have to serialize this exactly like OrganizationMember does? this is a whole new endpoint and we'll have to create a new FE object to handle this anyway.

it could make it easier to serialize the fields directly

@mifu67 mifu67 requested a review from cathteng March 20, 2025 06:55
Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 96.10390% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/testutils/factories.py 77.77% 2 Missing ⚠️
src/sentry/models/organizationmemberinvite.py 94.11% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #87475   +/-   ##
=======================================
  Coverage   87.74%   87.74%           
=======================================
  Files        9891     9893    +2     
  Lines      560952   561028   +76     
  Branches    22129    22129           
=======================================
+ Hits       492186   492255   +69     
- Misses      68362    68369    +7     
  Partials      404      404           

@@ -50,6 +52,32 @@ def generate_token():
return secrets.token_hex(nbytes=32)


# Causes a circular import error when importing
# from sentry.api.serializers.models.organization_member.response
_OrganizationMemberFlags = TypedDict(
Copy link
Member

Choose a reason for hiding this comment

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

do we have to serialize this exactly like OrganizationMember does? this is a whole new endpoint and we'll have to create a new FE object to handle this anyway.

it could make it easier to serialize the fields directly

Comment on lines +159 to +161
if self.token_expires_at > timezone.now():
return False
return True
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
if self.token_expires_at > timezone.now():
return False
return True
return self.token_expires_at <= timezone.now():


def test_simple(self):
member_invite = self.create_member_invite(organization=self.org, email=self.email)
result = serialize(member_invite, None, OrganizationMemberInviteSerializer())
Copy link
Member

Choose a reason for hiding this comment

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

hmm i was able to get it to pass by omitting the serializer

"member-limit:restricted": member_invite.member_limit_restricted,
"partnership:restricted": member_invite.partnership_restricted,
},
"teams": {},
Copy link
Member

Choose a reason for hiding this comment

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

the teams and team roles can be assigned via the UI, it could be that we're not testing it explicitly. we'll need to decide how this data will be stored here, in the existing system we appear to make the objects (OrganizationMemberTeam) but we won't be able to do that here

we could store the role and team as a mapping of team_id: role perhaps (i would suggest id instead of slug because slug is mutable)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants