diff --git a/backend/EduLite/courses/admin.py b/backend/EduLite/courses/admin.py index 6d65fa1..09499c5 100644 --- a/backend/EduLite/courses/admin.py +++ b/backend/EduLite/courses/admin.py @@ -1,66 +1,82 @@ from django.contrib import admin -from .models import Course, CourseModule, CourseMembership, CourseChatRoom +from django.core.exceptions import ValidationError + +from .models import Course, CourseChatRoom, CourseMembership, CourseModule # --- Inlines: Making the UI much faster to use --- # These let us edit modules and members directly inside the Course page, # so we don't have to jump around between different pages. + class CourseModuleInline(admin.TabularInline): """Allows adding/editing modules directly on the Course detail page.""" + model = CourseModule - fields = ('order', 'title', 'content_type', 'object_id') + fields = ("order", "title", "content_type", "object_id") extra = 1 # Shows one empty row by default for quick adding + class CourseMembershipInline(admin.TabularInline): """Lets us manage students/teachers directly from the Course page.""" + model = CourseMembership - fields = ('user', 'role', 'status') + fields = ("user", "role", "status") extra = 1 # --- Model Admins: Crafting the Dashboard experience --- + @admin.register(Course) class CourseAdmin(admin.ModelAdmin): # This controls the "Spreadsheet" view (the list of all courses) - list_display = ('title', 'visibility', 'is_active', 'start_date', 'subject') - + list_display = ("title", "visibility", "is_active", "start_date", "subject") + # Adding filters to the right sidebar so admins can drill down - list_filter = ('visibility', 'is_active', 'subject', 'language', 'country') - + list_filter = ("visibility", "is_active", "subject", "language", "country") + # Let's make it searchable by title and the outline text - search_fields = ('title', 'outline') - + search_fields = ("title", "outline") + # Grouping the fields into "buckets" to make the form less overwhelming fieldsets = ( - ("Core Information", { - 'fields': ('title', 'outline', 'subject', 'language', 'country') - }), - ("Access Control", { - 'fields': ('visibility', 'is_active') - }), - ("Timeline", { - 'fields': ('start_date', 'end_date') - }), + ( + "Core Information", + {"fields": ("title", "outline", "subject", "language", "country")}, + ), + ("Access Control", {"fields": ("visibility", "is_active")}), + ("Timeline", {"fields": ("start_date", "end_date")}), ) - + # Hooking up the inlines we defined above inlines = [CourseModuleInline, CourseMembershipInline] + def save_related(self, request, form, formsets, change): + """Enforce that every course has at least one enrolled teacher after saving inlines.""" + super().save_related(request, form, formsets, change) + course = form.instance + has_teacher = CourseMembership.objects.filter( + course=course, role="teacher", status="enrolled" + ).exists() + if not has_teacher: + raise ValidationError("A course must have at least one enrolled teacher.") + @admin.register(CourseMembership) class CourseMembershipAdmin(admin.ModelAdmin): """A dedicated view for managing user enrollments.""" - list_display = ('user', 'course', 'role', 'status') - list_filter = ('role', 'status', 'course') - search_fields = ('user__username', 'course__title') + + list_display = ("user", "course", "role", "status") + list_filter = ("role", "status", "course") + search_fields = ("user__username", "course__title") @admin.register(CourseChatRoom) class CourseChatRoomAdmin(admin.ModelAdmin): """Specific settings for the course chat functionality.""" - list_display = ('course', 'chatroom', 'created_by') - list_filter = ('course',) - + + list_display = ("course", "chatroom", "created_by") + list_filter = ("course",) + # Security: We don't want admins accidentally changing who created a room - readonly_fields = ('created_by',) \ No newline at end of file + readonly_fields = ("created_by",) diff --git a/backend/EduLite/courses/models.py b/backend/EduLite/courses/models.py index 8a54dab..05dcd71 100644 --- a/backend/EduLite/courses/models.py +++ b/backend/EduLite/courses/models.py @@ -211,13 +211,15 @@ class Meta: def clean(self) -> None: super().clean() - # Checking if the user is already a member of the course - if ( - CourseMembership.objects.filter(user=self.user, course=self.course) - .exclude(pk=self.pk) - .exists() - ): - raise ValidationError("User is already a member of the course") + # Skip duplicate check if either instance is unsaved (no PK yet). + # The UniqueConstraint on ["user", "course"] handles this at the DB level. + if self.user_id and self.course_id: + if ( + CourseMembership.objects.filter(user=self.user, course=self.course) + .exclude(pk=self.pk) + .exists() + ): + raise ValidationError("User is already a member of the course") # Checking state match the role if self.role != "student" and self.status == "pending": diff --git a/backend/EduLite/courses/tests/test_CourseMembership.py b/backend/EduLite/courses/tests/test_CourseMembership.py index e659331..74d9fda 100644 --- a/backend/EduLite/courses/tests/test_CourseMembership.py +++ b/backend/EduLite/courses/tests/test_CourseMembership.py @@ -2,9 +2,10 @@ # Tests for the CourseMembership model from datetime import datetime, timedelta -from django.test import TestCase + from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError +from django.test import TestCase from ..models import Course, CourseMembership @@ -181,3 +182,50 @@ def test_course_delete_cascade(self) -> None: membership_id = course_membership.id self.course1.delete() self.assertFalse(CourseMembership.objects.filter(pk=membership_id).exists()) + + def test_clean_with_unsaved_course_does_not_crash(self) -> None: + """ + Test that clean() does not crash when the course is unsaved (no PK). + This happens when creating a course with an inline membership in admin. + """ + unsaved_course = Course(title="Unsaved Course") + membership = CourseMembership( + user=self.user1, + course=unsaved_course, + role="teacher", + status="enrolled", + ) + # Should not raise ValueError about unsaved model instances + membership.clean() + + def test_clean_with_unsaved_user_does_not_crash(self) -> None: + """ + Test that clean() does not crash when the user is unsaved (no PK). + """ + unsaved_user = User(username="unsaved", email="unsaved@example.com") + membership = CourseMembership( + user=unsaved_user, + course=self.course1, + role="student", + status="enrolled", + ) + # Should not raise ValueError about unsaved model instances + membership.clean() + + def test_clean_still_catches_duplicates_when_saved(self) -> None: + """ + Test that clean() still catches duplicate memberships when both + user and course are saved (have PKs). + """ + CourseMembership.objects.create( + user=self.user1, + course=self.course1, + role="student", + ) + duplicate = CourseMembership( + user=self.user1, + course=self.course1, + role="teacher", + ) + with self.assertRaises(ValidationError): + duplicate.clean() diff --git a/backend/EduLite/courses/tests/test_admin.py b/backend/EduLite/courses/tests/test_admin.py new file mode 100644 index 0000000..85df2b7 --- /dev/null +++ b/backend/EduLite/courses/tests/test_admin.py @@ -0,0 +1,154 @@ +# courses/tests/test_admin.py +# Tests for CourseAdmin teacher requirement enforcement + +from datetime import datetime + +from django.contrib.admin.sites import AdminSite +from django.contrib.auth import get_user_model +from django.core.exceptions import ValidationError +from django.test import RequestFactory, TestCase + +from ..admin import CourseAdmin +from ..models import Course, CourseMembership + +User = get_user_model() + + +class _MockForm: + """Mock form for testing save_related. Needs save_m2m for Django admin.""" + + def __init__(self, instance): + self.instance = instance + + def save_m2m(self): + pass + + +class TestCourseAdminTeacherRequirement(TestCase): + """Tests that CourseAdmin enforces at least one enrolled teacher.""" + + def setUp(self): + self.site = AdminSite() + self.admin = CourseAdmin(Course, self.site) + self.factory = RequestFactory() + self.superuser = User.objects.create_superuser( + username="admin", + email="admin@example.com", + password="adminpass123", + ) + self.teacher_user = User.objects.create_user( + username="teacher", + email="teacher@example.com", + password="pass123", + ) + self.student_user = User.objects.create_user( + username="student", + email="student@example.com", + password="pass123", + ) + + def test_save_related_raises_when_no_teacher(self): + """Creating a course without a teacher membership should raise ValidationError.""" + course = Course.objects.create( + title="No Teacher Course", + visibility="public", + start_date=datetime(2025, 1, 1), + ) + + request = self.factory.post("/admin/courses/course/add/") + request.user = self.superuser + + with self.assertRaises(ValidationError) as ctx: + self.admin.save_related( + request, _MockForm(course), formsets=[], change=False + ) + + self.assertIn("at least one enrolled teacher", str(ctx.exception)) + + def test_save_related_succeeds_with_teacher(self): + """Creating a course with a teacher membership should succeed.""" + course = Course.objects.create( + title="Has Teacher Course", + visibility="public", + start_date=datetime(2025, 1, 1), + ) + CourseMembership.objects.create( + user=self.teacher_user, + course=course, + role="teacher", + status="enrolled", + ) + + request = self.factory.post("/admin/courses/course/add/") + request.user = self.superuser + + # Should not raise + self.admin.save_related(request, _MockForm(course), formsets=[], change=False) + + def test_save_related_raises_when_only_students(self): + """A course with only student memberships should raise ValidationError.""" + course = Course.objects.create( + title="Students Only Course", + visibility="public", + start_date=datetime(2025, 1, 1), + ) + CourseMembership.objects.create( + user=self.student_user, + course=course, + role="student", + status="enrolled", + ) + + request = self.factory.post("/admin/courses/course/add/") + request.user = self.superuser + + with self.assertRaises(ValidationError): + self.admin.save_related( + request, _MockForm(course), formsets=[], change=False + ) + + def test_save_related_raises_when_teacher_not_enrolled(self): + """A teacher with 'invited' status should not count as an enrolled teacher.""" + course = Course.objects.create( + title="Invited Teacher Course", + visibility="public", + start_date=datetime(2025, 1, 1), + ) + CourseMembership.objects.create( + user=self.teacher_user, + course=course, + role="teacher", + status="invited", + ) + + request = self.factory.post("/admin/courses/course/add/") + request.user = self.superuser + + with self.assertRaises(ValidationError): + self.admin.save_related( + request, _MockForm(course), formsets=[], change=False + ) + + def test_save_related_blocks_removing_last_teacher(self): + """Removing the last teacher from an existing course should raise ValidationError.""" + course = Course.objects.create( + title="Existing Course", + visibility="public", + start_date=datetime(2025, 1, 1), + ) + membership = CourseMembership.objects.create( + user=self.teacher_user, + course=course, + role="teacher", + status="enrolled", + ) + # Simulate removing the teacher (delete the membership before save_related check) + membership.delete() + + request = self.factory.post(f"/admin/courses/course/{course.pk}/change/") + request.user = self.superuser + + with self.assertRaises(ValidationError): + self.admin.save_related( + request, _MockForm(course), formsets=[], change=True + )