Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(organizations): add endpoints to handle organization members TASK-963 #5235
feat(organizations): add endpoints to handle organization members TASK-963 #5235
Changes from 2 commits
e17f6a7
024f911
0401fc4
1dec6eb
3c174df
f432404
d876fbc
953b716
f8cb95b
3d0372a
906be3d
d122576
94b47cb
01c189c
89db1d1
988b10b
e5abfa0
def1a7a
c2cf803
d706056
0e28408
4d67c50
d2fa51d
5c0ef26
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the wrong field. You need to retrieve
User.extra_details.data['name']
. BTW in that case, the field should beuser__extra_details__name
.cc @jamesrkiger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you need to get if the user has MFA enabled. (e.g.
MfaMethod.objects.filter(user=obj, is_active=True).exists()
). Ensure to optimize this for a list like you did for role ;-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future-proof validaiton, please add
mmo_override=True
This can be simplied with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invited_user
is not used at all for the moment, right? Let's remove until we implement invitations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All your tests are done with
self.client == self.owner
, but we need to validate permissions for regular members, admins and anonymous. Let's those tests as soon as #5218 is merged. I will tag this PRblocked by
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out! I've added the role-based validation tests for organization member permissions.
One thing I still need to figure out is that I have added a new permission class and tried to use obj to retrieve the user role using the optimized approach I wrote in
get_queryset()
. It works fine when I test the endpoints in Postman or using a curl. However, for the tests, I'm unable to fetch the actual role, and it defaults to themember
role. Do you have any suggestions for this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename the class just to match what it is paginating (i.e.: organization members)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
10
a requirement?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 is not a requirement defined in the task. I need to check with you or the team to determine the appropriate value for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rajpatel24 Can we change this to limit offset pagination? Our table components on the frontend currently assume limit offset pagination and @noliveleger said it shouldn't cause any problems to use that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jamesrkiger I have removed the custom pagination class for the organization members API. It will now use
LimitOffsetPagination
by default.