feat(execom): added campus execom management with role assignments #2559
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a campus execom (executive committee) management system that allows tracking of execom members and their role assignments for campus organizations.
- Added
CampusExecommodel to store execom member-role-campus relationships - Created REST API endpoints for viewing, adding, and removing execom members
- Implemented URL routing for campus execom management operations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 19 comments.
| File | Description |
|---|---|
| db/organization.py | Adds CampusExecom model with user-campus-role relationship tracking |
| api/urls.py | Adds URL routes for campus execom list/detail endpoints |
| api/campus_execom_views.py | Implements APIView with GET, POST, and DELETE methods for execom management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| class CampusExecom(models.Model): | ||
| id = models.CharField(primary_key=True, max_length=36, default=uuid.uuid4) |
There was a problem hiding this comment.
The UUID default should be callable (uuid.uuid4) not called (uuid.uuid4()). This is inconsistent with other models like State, Zone, and District in the same file which use uuid.uuid4 without parentheses. Using uuid.uuid4() means the same UUID will be used for all instances created during the same module import, while uuid.uuid4 (callable) generates a new UUID for each instance.
| import debug_toolbar | ||
| from django.urls import path, include | ||
|
|
||
| from.campus_execom_views import CampusExecomView |
There was a problem hiding this comment.
Missing space after dot in import statement. Should be 'from .' instead of 'from.'
| from.campus_execom_views import CampusExecomView | |
| from .campus_execom_views import CampusExecomView |
| path('campus/<str:campus_id>/execom/',CampusExecomView.as_view()), | ||
| path('campus/<str:campus_id>/execom/<str:uid>/',CampusExecomView.as_view()), |
There was a problem hiding this comment.
Missing space after comma in path function call. This is inconsistent with Python style guidelines (PEP 8) which recommend a space after commas.
| path('campus/<str:campus_id>/execom/',CampusExecomView.as_view()), | |
| path('campus/<str:campus_id>/execom/<str:uid>/',CampusExecomView.as_view()), | |
| path('campus/<str:campus_id>/execom/', CampusExecomView.as_view()), | |
| path('campus/<str:campus_id>/execom/<str:uid>/', CampusExecomView.as_view()), |
| if CampusExecom.objects.filter(campus_id=campus_id, user_id=user_id).exists(): | ||
| return Response({'message': 'User is already in Execom'}, status=400) |
There was a problem hiding this comment.
The existence check only prevents duplicates at the application level but not at the database level. This creates a race condition where two concurrent requests could both pass the check and create duplicate records. The unique constraint should be enforced at the database level through the model's Meta class.
| path('campus/<str:campus_id>/execom/',CampusExecomView.as_view()), | ||
| path('campus/<str:campus_id>/execom/<str:uid>/',CampusExecomView.as_view()), |
There was a problem hiding this comment.
Missing space after comma in path function call. This is inconsistent with Python style guidelines (PEP 8) which recommend a space after commas.
| path('campus/<str:campus_id>/execom/',CampusExecomView.as_view()), | |
| path('campus/<str:campus_id>/execom/<str:uid>/',CampusExecomView.as_view()), | |
| path('campus/<str:campus_id>/execom/', CampusExecomView.as_view()), | |
| path('campus/<str:campus_id>/execom/<str:uid>/', CampusExecomView.as_view()), |
| campus_id=campus_id, | ||
| role=role | ||
| ) | ||
| return Response({'message': 'Member added successfully'}) |
There was a problem hiding this comment.
Missing HTTP status code in success response. Following REST API best practices and consistency with other endpoints in the codebase, the response should include status=201 to indicate resource creation.
| # 3. REMOVE MEMBER (DELETE) | ||
| def delete(self, request, campus_id, uid): | ||
| CampusExecom.objects.filter(id=uid).delete() | ||
| return Response({'message': 'Member removed successfully'}) No newline at end of file |
There was a problem hiding this comment.
Missing HTTP status code in success response. Following REST API best practices, the DELETE endpoint should return status=204 (No Content) for successful deletion or status=200 with a message.
| return Response({'message': 'Member removed successfully'}) | |
| return Response({'message': 'Member removed successfully'}, status=200) |
| def delete(self, request, campus_id, uid): | ||
| CampusExecom.objects.filter(id=uid).delete() |
There was a problem hiding this comment.
Inconsistent parameter naming. The URL parameter is named 'uid' but it represents an execom member ID. Consider renaming to 'execom_id' or 'member_id' for clarity and consistency with the domain model.
| def delete(self, request, campus_id, uid): | |
| CampusExecom.objects.filter(id=uid).delete() | |
| def delete(self, request, campus_id, execom_id): | |
| CampusExecom.objects.filter(id=execom_id).delete() |
| @@ -0,0 +1,38 @@ | |||
| from rest_framework.views import APIView | |||
| from rest_framework.response import Response | |||
| from db.organization import CampusExecom, Organization | |||
There was a problem hiding this comment.
Import of 'Organization' is not used.
| from db.organization import CampusExecom, Organization | |
| from db.organization import CampusExecom |
| from rest_framework.views import APIView | ||
| from rest_framework.response import Response | ||
| from db.organization import CampusExecom, Organization | ||
| from db.user import User |
There was a problem hiding this comment.
Import of 'User' is not used.
| from db.user import User |
1 participant
No description provided.