Fix CourseMembership.clean() crash on unsaved course & enforce teacher requirement

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',)
+    readonly_fields = ("created_by",)

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":

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

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