feat(organizations): add endpoints to handle organization members TASK-963 #5235

rajpatel24 · 2024-11-06T19:56:27Z

🗒️ Checklist

run linter locally
update all related docs (API, README, inline, etc.), if any
draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
tag PR: at least frontend or backend unless it's global
fill in the template below and delete template comments
review thyself: read the diff and repro the preview as written
open PR & confirm that CI passes
request reviewers, if needed
delete this section before merging

📣 Summary

Enhance the organization member management API to support role updates and member removal via PATCH and DELETE requests, excluding member creation.

📖 Description

This update introduces endpoints for managing organization members:
- GET /api/v2/organizations/<org-id>/members/ to list all members in an organization.
- PATCH /api/v2/organizations/<org-id>/members/<username>/ to update a member's role (e.g., promote to admin).
- DELETE /api/v2/organizations/<org-id>/members/<username>/ to remove a member from the organization.

Note: Creating members is not supported via this endpoint. Roles are restricted to 'admin' or 'member'. Including the details of invited members (those who have not yet joined the organization) is not covered in this update.

👷 Description for instance maintainers

This update provides new functionality to manage organization members:
- Role updates for members (promotion/demotion between 'admin' and 'member').
- Member removal from an organization.
- The endpoints are optimized to use database joins to fetch member roles efficiently without excessive database queries, ensuring minimal load.

👀 Preview steps

ℹ️ Have an account and an organization with multiple members.
Use the GET method to list the members of the organization.
Use the PATCH method to update a member's role (e.g., promote a user to admin).
Use the DELETE method to remove a member from the organization.
🟢 Notice the endpoints behave as expected, with valid role updates and member removal.

💭 Notes

The code uses database joins to optimize role determination for members, avoiding inefficient role lookups.
Role validation restricts role assignments to 'admin' or 'member' only.
Including invited members' details is not covered in this update. This will be implemented in a future update.

…e-endpoints-to-handle-org-members

noliveleger · 2024-11-08T14:33:53Z

kpi/paginators.py

@@ -142,3 +142,10 @@ class TinyPaginated(PageNumberPagination):
    Same as Paginated with a small page size
    """
    page_size = 50
+
+
+class OrganizationPagination(PageNumberPagination):


I would rename the class just to match what it is paginating (i.e.: organization members)

noliveleger · 2024-11-08T14:34:23Z

kpi/paginators.py

+    """
+    Pagination class for Organization
+    """
+    page_size = 10


Is 10 a requirement?

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.

@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.

@jamesrkiger I have removed the custom pagination class for the organization members API. It will now use LimitOffsetPagination by default.

noliveleger · 2024-11-08T14:37:13Z

kobo/apps/organizations/serializers.py

+        ]
+
+    def get_has_mfa_enabled(self, obj):
+        return config.MFA_ENABLED


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 ;-)

noliveleger · 2024-11-08T14:42:25Z

kobo/apps/organizations/tests/test_organization_members_api.py

+    URL_NAMESPACE = URL_NAMESPACE
+
+    def setUp(self):
+        self.organization = baker.make(Organization, id='org_12345')


For future-proof validaiton, please add mmo_override=True
This can be simplied with

self.organization = baker.make(Organization, id='org_12345', mmo_override=True) self.owner_user = baker.make(User, username='owner') self.member_user = baker.make(User, username='member') self.invited_user = baker.make(User, username='invited') self.organization.add_user(self.owner_user) # first user added to org, is always the owner and an admin self.organization.add_user(self.member_user, is_admin=False)

noliveleger · 2024-11-08T14:45:04Z

kobo/apps/organizations/tests/test_organization_members_api.py

+        self.organization = baker.make(Organization, id='org_12345')
+        self.owner_user = baker.make(User, username='owner')
+        self.member_user = baker.make(User, username='member')
+        self.invited_user = baker.make(User, username='invited')


invited_user is not used at all for the moment, right? Let's remove until we implement invitations.

noliveleger · 2024-11-08T14:47:12Z

kobo/apps/organizations/tests/test_organization_members_api.py

+    def test_list_members(self):
+        response = self.client.get(self.list_url)
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertIn(
+            'owner',
+            [member['user__username'] for member in response.data.get('results')]
+        )
+        self.assertIn(
+            'member',
+            [member['user__username'] for member in response.data.get('results')]
+        )
+
+    def test_retrieve_member_details(self):
+        response = self.client.get(self.detail_url('member'))
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data['user__username'], 'member')
+        self.assertEqual(response.data['role'], 'member')
+
+    def test_update_member_role(self):
+        data = {'role': 'admin'}
+        response = self.client.patch(self.detail_url('member'), data)
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.data['role'], 'admin')
+
+    def test_delete_member(self):
+        response = self.client.delete(self.detail_url('member'))
+        self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT)
+        # Confirm deletion
+        response = self.client.get(self.detail_url('member'))
+        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)


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 PR blocked by

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 the member role. Do you have any suggestions for this?

kobo/apps/organizations/serializers.py

kobo/apps/organizations/views.py

noliveleger · 2024-11-08T15:05:04Z

This PR is blocked by #5218

magicznyleszek · 2024-11-12T12:59:09Z

@noliveleger @rajpatel24 this is no longer blocked, right?

… query for user mfa details

…omprehensive tests

…ation user table and enhance test coverage

…n members

kobo/apps/organizations/permissions.py

kobo/apps/organizations/serializers.py

kobo/apps/organizations/tests/test_organization_members_api.py

noliveleger · 2024-11-20T20:20:57Z

kpi/paginators.py

+class OrganizationMembersPagination(PageNumberPagination):
+    """
+    Pagination class for Organization Members
+    """
+    page_size_query_param = 'page_size'
+
+


You can delete this class, not needed anymore

Done. It will use LimitOffsetPagination by default

noliveleger · 2024-11-20T20:23:44Z

kobo/apps/organizations/serializers.py

+        source='user.date_joined', format='%Y-%m-%dT%H:%M:%SZ'
+    )
+    user__username = serializers.ReadOnlyField(source='user.username')
+    user__name = serializers.ReadOnlyField(source='user.get_full_name')


I think that's the wrong field. You need to retrieve User.extra_details.data['name'] . BTW in that case, the field should be user__extra_details__name .

cc @jamesrkiger

noliveleger · 2024-11-20T20:25:55Z

kobo/apps/organizations/views.py

@@ -79,10 +86,12 @@ class OrganizationViewSet(viewsets.ModelViewSet):
    queryset = Organization.objects.all()
    serializer_class = OrganizationSerializer
    lookup_field = 'id'
-    permission_classes = (IsAuthenticated, IsOrgAdminOrReadOnly)
+    permission_classes = [HasOrgRolePermission]
    pagination_class = AssetUsagePagination


Asked by the front-end team, let's use LimitOffsetPagination instead.

Okay, I removed AssetUsagePagination. It will now use LimitOffsetPagination by default for /organizations and /organizations/{org_id}/members.

noliveleger · 2024-11-20T20:39:06Z

kobo/apps/organizations/views.py

+    def destroy(self, request, *args, **kwargs):
+        """
+        Delete an organization member and their associated user account
+        """
+        instance = self.get_object()
+        user = instance.user
+        instance.delete()
+        user.delete()
+        return Response(status=status.HTTP_204_NO_CONTENT)


You can remove this method entirely. No need to overload but right now, we want to delete the OrganizationUser only. Do not touch the User object.

…le pagination with LimitOffsetPagination

…delete requests TASK-964 (#5274) ### 📣 Summary Restricted POST and DELETE methods for `organizations` endpoint to prevent unintended changes. ### 📖 Description Previously, the organizations endpoint allowed POST and DELETE requests, which could lead to accidental changes or deletions. This update restricts these methods, ensuring that organizations data remains intact and secure. ### 👀 Preview steps - Send a GET request to the organizations endpoint to retrieve a list of organizations. - Attempt to send a POST request to create a new organization (this should fail). - Attempt to send a DELETE request to delete an existing organization (this should fail). - Send a PATCH request to update an existing organization (this should succeed). ### 💭 Notes - This change is a security enhancement to prevent accidental or malicious modifications to organizations data. - Tests have been updated to reflect the new behaviour.

… to improve performance

noliveleger

LGTM

For the record: The complex query took only few millisecond on a 500 members org.

rajpatel24 added the Back end label Nov 6, 2024

rajpatel24 requested review from jnm and noliveleger as code owners November 6, 2024 19:56

rajpatel24 force-pushed the task-963-create-endpoints-to-handle-org-members branch from dcaab9f to 7533999 Compare November 6, 2024 20:10

rajpatel24 removed the request for review from jnm November 6, 2024 20:11

Add endpoints to handle organization members

e17f6a7

rajpatel24 force-pushed the task-963-create-endpoints-to-handle-org-members branch from 7533999 to e17f6a7 Compare November 6, 2024 20:13

Merge branch 'main' of github.com:kobotoolbox/kpi into task-963-creat…

024f911

…e-endpoints-to-handle-org-members

rajpatel24 changed the title ~~feat(Organizations): Add endpoints to handle organization members TASK-963~~ feat(organizations): add endpoints to handle organization members TASK-963 Nov 8, 2024

noliveleger requested changes Nov 8, 2024

View reviewed changes

kobo/apps/organizations/serializers.py Show resolved Hide resolved

kobo/apps/organizations/views.py Outdated Show resolved Hide resolved

noliveleger assigned rajpatel24 Nov 8, 2024

noliveleger added the blocked label Nov 8, 2024

noliveleger removed the blocked label Nov 12, 2024

rajpatel24 added 2 commits November 13, 2024 15:33

Refactor organization member API to eliminate redundancy and optimize…

0401fc4

… query for user mfa details

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

1dec6eb

rajpatel24 force-pushed the task-963-create-endpoints-to-handle-org-members branch from 05e5c5f to c25d807 Compare November 14, 2024 15:46

Add role-based validation tests for organization member permissions

3c174df

rajpatel24 force-pushed the task-963-create-endpoints-to-handle-org-members branch from c25d807 to 3c174df Compare November 14, 2024 19:51

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

f432404

rajpatel24 force-pushed the task-963-create-endpoints-to-handle-org-members branch from 315941e to 7c580e7 Compare November 14, 2024 20:20

Revert unintended change and fix linting issue

d876fbc

rajpatel24 force-pushed the task-963-create-endpoints-to-handle-org-members branch from 7c580e7 to d876fbc Compare November 14, 2024 20:27

rajpatel24 requested a review from noliveleger November 14, 2024 20:39

rajpatel24 added 4 commits November 16, 2024 00:10

Refactor organization members API with updated permission logic and c…

953b716

…omprehensive tests

Refactor organization members API with updated permission logic and c…

f8cb95b

…omprehensive tests

Resolve merge conflicts

3d0372a

Fix failing tests

906be3d

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

d122576

magicznyleszek mentioned this pull request Nov 19, 2024

feat(organizations): members table TASK-980 #5261

Merged

magicznyleszek and others added 6 commits November 19, 2024 03:00

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

94b47cb

Update delete logic to remove user from user table along with organiz…

01c189c

…ation user table and enhance test coverage

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

89db1d1

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

988b10b

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

e5abfa0

Refactor permissions to block external users from listing organizatio…

def1a7a

…n members

noliveleger requested changes Nov 20, 2024

View reviewed changes

noliveleger reviewed Nov 20, 2024

View reviewed changes

magicznyleszek and others added 3 commits November 20, 2024 23:03

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

c2cf803

Refactor serializer to retrieve user name from extra details and enab…

d706056

…le pagination with LimitOffsetPagination

rajpatel24 requested a review from noliveleger November 21, 2024 09:43

Optimize organization members API by prefetching related user details…

4d67c50

… to improve performance

noliveleger approved these changes Nov 21, 2024

View reviewed changes

rajpatel24 added 2 commits November 22, 2024 11:16

Merge branch 'main' into task-963-create-endpoints-to-handle-org-members

d2fa51d

Update organization members API doc

5c0ef26

rajpatel24 merged commit 79e33e8 into main Nov 22, 2024
7 checks passed

rajpatel24 deleted the task-963-create-endpoints-to-handle-org-members branch November 22, 2024 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(organizations): add endpoints to handle organization members TASK-963 #5235

feat(organizations): add endpoints to handle organization members TASK-963 #5235

rajpatel24 commented Nov 6, 2024 •

edited

Loading

noliveleger Nov 8, 2024

noliveleger Nov 8, 2024

rajpatel24 Nov 14, 2024

jamesrkiger Nov 19, 2024

rajpatel24 Nov 21, 2024

noliveleger Nov 8, 2024

noliveleger Nov 8, 2024

noliveleger Nov 8, 2024

noliveleger Nov 8, 2024

rajpatel24 Nov 14, 2024 •

edited

Loading

noliveleger commented Nov 8, 2024

magicznyleszek commented Nov 12, 2024

noliveleger Nov 20, 2024

rajpatel24 Nov 21, 2024

noliveleger Nov 20, 2024

noliveleger Nov 20, 2024

rajpatel24 Nov 21, 2024 •

edited

Loading

noliveleger Nov 20, 2024 •

edited

Loading

noliveleger left a comment

feat(organizations): add endpoints to handle organization members TASK-963 #5235

feat(organizations): add endpoints to handle organization members TASK-963 #5235

Conversation

rajpatel24 commented Nov 6, 2024 • edited Loading

🗒️ Checklist

📣 Summary

📖 Description

👷 Description for instance maintainers

👀 Preview steps

💭 Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajpatel24 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

noliveleger commented Nov 8, 2024

magicznyleszek commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajpatel24 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

noliveleger Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

noliveleger left a comment

Choose a reason for hiding this comment

rajpatel24 commented Nov 6, 2024 •

edited

Loading

rajpatel24 Nov 14, 2024 •

edited

Loading

rajpatel24 Nov 21, 2024 •

edited

Loading

noliveleger Nov 20, 2024 •

edited

Loading