Feat/user profile enhancement#2718
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the dashboard backend with (1) an enhanced user profile surface (bio/projects/experience) and (2) a new Campus Execom management feature (model + endpoints), both implemented via unmanaged Django models and DRF APIs.
Changes:
- Added
UserProfilemodel plus a newUserProfileEnhancedAPIand serializers to read/update extended profile fields. - Added
CampusExecommodel, serializers, and new campus routes/views to list/create/update/delete execom members per campus. - Added an implementation guide document for the Campus Execom feature and rewired profile URL routing to the enhanced profile endpoint.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| db/user.py | Adds unmanaged UserProfile model for extended profile fields. |
| db/organization.py | Adds unmanaged CampusExecom model with constraints/indexes for campus execom membership. |
| api/dashboard/profile/urls.py | Routes user-profile/ to the new enhanced profile API. |
| api/dashboard/profile/profile_view.py | Implements GET/PATCH for enhanced profile data via UserProfile. |
| api/dashboard/profile/profile_serializer.py | Adds serializers for the new UserProfile model (but currently conflicts with existing serializer naming). |
| api/dashboard/campus/urls.py | Adds campus execom routes scoped by org_id. |
| api/dashboard/campus/serializers.py | Adds serializers for campus execom list/create flows. |
| api/dashboard/campus/campus_views.py | Adds execom list/create/delete API views. |
| CAMPUS_EXECOM_IMPLEMENTATION.md | Documents the new campus execom system and endpoints. |
💡 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()) | ||
| org = models.ForeignKey(Organization, on_delete=models.CASCADE, related_name='campus_execom_org') | ||
| user = models.ForeignKey(User, on_delete=models.CASCADE, related_name='campus_execom_user') | ||
| role = models.ForeignKey(Role, on_delete=models.SET_NULL, null=True, related_name='campus_execom_role') | ||
| updated_by = models.ForeignKey(User, on_delete=models.SET(settings.SYSTEM_ADMIN_ID), db_column='updated_by', related_name='campus_execom_updated_by') |
There was a problem hiding this comment.
CampusExecom references Role, but Role is not imported in this module. Importing db.organization will raise NameError at class definition time. Add the appropriate import (e.g., from db.user import Role or from .user import Role).
| class CampusExecom(models.Model): | ||
| id = models.CharField(primary_key=True, max_length=36, default=uuid.uuid4()) | ||
| org = models.ForeignKey(Organization, on_delete=models.CASCADE, related_name='campus_execom_org') | ||
| user = models.ForeignKey(User, on_delete=models.CASCADE, related_name='campus_execom_user') |
There was a problem hiding this comment.
default=uuid.uuid4() is evaluated once at import time, so new CampusExecom rows created through Django will all reuse the same primary key and fail with duplicate PK errors. Use a callable default (e.g., uuid.uuid4 or lambda: str(uuid.uuid4())) instead of calling it.
| class Meta: | ||
| managed = False | ||
| db_table = 'campus_execom' | ||
| unique_together = ('org', 'user') | ||
| indexes = [ | ||
| models.Index(fields=['org', 'user']), | ||
| models.Index(fields=['org']), | ||
| ] |
There was a problem hiding this comment.
unique_together = ('org', 'user') is not the correct shape for Django’s unique_together (it should be a tuple/list of tuples, e.g. (('org', 'user'),) or [('org','user')]). As written, Django will normalize it incorrectly and the constraint won’t behave as intended.
| class UserProfileSerializer(serializers.ModelSerializer): | ||
| user_id = serializers.CharField(source="user.id", read_only=True) | ||
| full_name = serializers.CharField(source="user.full_name", read_only=True) | ||
| email = serializers.CharField(source="user.email", read_only=True) | ||
| muid = serializers.CharField(source="user.muid", read_only=True) | ||
| profile_pic = serializers.SerializerMethodField() | ||
|
|
||
| class Meta: | ||
| model = UserProfile |
There was a problem hiding this comment.
This file defines UserProfileSerializer twice. The second definition overwrites the existing UserProfileSerializer (used for the main User profile response), which will break endpoints that still expect the original serializer. Rename the new serializer (e.g., UserProfileEnhancedSerializer) and update usages accordingly.
| def validate_projects(self, value): | ||
| if not isinstance(value, list): | ||
| raise serializers.ValidationError("Projects must be a list") | ||
|
|
||
| for project in value: | ||
| if not isinstance(project, dict): | ||
| raise serializers.ValidationError("Each project must be an object") | ||
|
|
||
| required_fields = ["title", "link", "description", "tags"] | ||
| for field in required_fields: | ||
| if field not in project: | ||
| raise serializers.ValidationError(f"Project must have '{field}' field") | ||
|
|
||
| return value | ||
|
|
||
| def validate_experience(self, value): | ||
| if not isinstance(value, list): | ||
| raise serializers.ValidationError("Experience must be a list") | ||
|
|
||
| for exp in value: | ||
| if not isinstance(exp, dict): | ||
| raise serializers.ValidationError("Each experience entry must be an object") | ||
|
|
||
| return value |
There was a problem hiding this comment.
validate_projects/validate_experience reject None, but the model fields are null=True. This makes it impossible to clear these fields via PATCH by sending null. Either disallow nulls at the model/serializer level, or accept None here (and configure the serializer fields with allow_null=True).
| class CampusExecomAPI(APIView): | ||
| """ | ||
| Campus Execom API | ||
|
|
||
| This API view allows authorized users to manage the Executive Committee | ||
| members of a campus (college). | ||
|
|
||
| Endpoints: | ||
| GET /api/campus/:id/execom - View all execom members | ||
| POST /api/campus/:id/execom - Add a new execom member | ||
| DELETE /api/campus/:id/execom/:uid - Remove an execom member | ||
| """ | ||
|
|
||
| authentication_classes = [CustomizePermission] | ||
|
|
||
| def get(self, request, org_id): |
There was a problem hiding this comment.
These execom management endpoints only check that the caller has a UserOrganizationLink to the campus. Other privileged campus actions in this module use @role_required(...) (e.g., role transfers). If execom management is intended to be admin-only, add a role restriction (Campus Lead / Lead Enabler) to prevent regular campus members from modifying execom.
| try: | ||
| execom = serializer.save() | ||
| response_serializer = serializers.CampusExecomSerializer(execom) | ||
| return CustomResponse( | ||
| response=response_serializer.data | ||
| ).get_success_response() | ||
| except Exception as e: | ||
| return CustomResponse( | ||
| general_message=str(e) | ||
| ).get_failure_response() |
There was a problem hiding this comment.
Catching a broad Exception and returning general_message=str(e) will leak internal error details to clients and makes responses unstable. Prefer handling expected exceptions (e.g., DoesNotExist, ValidationError) and return a generic failure message while logging the exception server-side.
| try: | ||
| execom.delete() | ||
| return CustomResponse( | ||
| general_message="Execom member removed successfully" | ||
| ).get_success_response() | ||
| except Exception as e: | ||
| return CustomResponse( | ||
| general_message=str(e) | ||
| ).get_failure_response() |
There was a problem hiding this comment.
Same issue here: returning general_message=str(e) can expose internal details. Use a stable client-facing message and log the exception instead.
| # Campus Execom Management System - Implementation Guide | ||
|
|
||
| ## Overview | ||
| This implementation adds a backend system to manage the campus Executive Committee (Execom), allowing admins to assign or remove users based on their roles. | ||
|
|
||
| ## Changes Made |
There was a problem hiding this comment.
PR title is about user profile enhancement, but this PR also adds a full Campus Execom management system (model/API/docs). Consider splitting into separate PRs or updating the PR title/description so reviewers can assess scope appropriately.
| ```python | ||
| class CampusExecom(models.Model): | ||
| id = models.CharField(primary_key=True, max_length=36, default=uuid.uuid4()) | ||
| org = models.ForeignKey(Organization, on_delete=models.CASCADE, related_name='campus_execom_org') | ||
| user = models.ForeignKey(User, on_delete=models.CASCADE, related_name='campus_execom_user') | ||
| role = models.ForeignKey(Role, on_delete=models.SET_NULL, null=True, related_name='campus_execom_role') | ||
| updated_by = models.ForeignKey(User, on_delete=models.SET(settings.SYSTEM_ADMIN_ID), db_column='updated_by', related_name='campus_execom_updated_by') | ||
| updated_at = models.DateTimeField(auto_now=True) | ||
| created_by = models.ForeignKey(User, on_delete=models.SET(settings.SYSTEM_ADMIN_ID), db_column='created_by', related_name='campus_execom_created_by') | ||
| created_at = models.DateTimeField(auto_now_add=True) |
There was a problem hiding this comment.
The implementation guide’s CampusExecom snippet uses default=uuid.uuid4() for the id, which will produce a single reused UUID (evaluated at import time) if copied into code. Update the doc snippet to use a callable default (no parentheses) to match the intended behavior.
- Add UserProfile model with bio, projects, and experience fields - Create serializers for profile read and update operations - Implement GET and PATCH endpoints for user profile - Add project object validation with required fields (title, link, description, tags) - Add audit trail for profile updates (created_by, updated_by) - Add URL routing for /api/v1/dashboard/profile/user-profile/ endpoint
6edb86f to
e75049f
Compare
No description provided.