diff --git a/accounts/admin.py b/accounts/admin.py index 79b8cc4..51ecfbc 100644 --- a/accounts/admin.py +++ b/accounts/admin.py @@ -2,13 +2,18 @@ from django import forms from django.contrib import admin - +from unfold.admin import ModelAdmin +from django.contrib.auth.admin import UserAdmin from .models import CustomUser, Token +from unfold.forms import UserCreationForm, AdminPasswordChangeForm, UserChangeForm -class CustomUserAdmin(admin.ModelAdmin): +class CustomUserAdmin(UserAdmin, ModelAdmin): search_fields = ["email"] change_form_template = 'loginas/change_form.html' + form = UserChangeForm + add_form = UserCreationForm + change_password_form = AdminPasswordChangeForm admin.site.register(CustomUser, CustomUserAdmin) diff --git a/course_management/settings.py b/course_management/settings.py index b505582..62ceb3e 100644 --- a/course_management/settings.py +++ b/course_management/settings.py @@ -16,7 +16,7 @@ from pathlib import Path import dj_database_url -from django.templatetags.static import static +from django.urls import reverse_lazy from django.utils.translation import gettext_lazy as _ @@ -319,4 +319,85 @@ "SITE_HEADER": _("Course Management"), "SITE_TITLE": _("Course Management"), "SITE_SYMBOL": "school", -} \ No newline at end of file + "SIDEBAR": { + "show_search": True, + "show_all_applications": True, + "navigation": [ + { + "title": _("Account"), + "items": [ + { + "title": "Users", + "icon": "person", + "link": reverse_lazy( + "admin:accounts_customuser_changelist" + ), + "permission": lambda request: request.user.is_superuser, + }, + { + "title": "Groups", + "icon": "groups", + "link": reverse_lazy( + "admin:auth_group_changelist" + ), + "permission": lambda request: request.user.is_superuser, + }, + { + "title": "Tokens", + "icon": "key", + "link": reverse_lazy( + "admin:accounts_token_changelist" + ), + "permission": lambda request: request.user.is_superuser, + }, + { + "title": "Email addresses", + "icon": "email", + "link": reverse_lazy( + "admin:account_emailaddress_changelist" + ), + "permission": lambda request: request.user.is_superuser, + }, + ], + }, + { + "title": _("Courses"), + "items": [ + { + "title": "Home", + "icon": "home", + "link": reverse_lazy("admin:index"), + }, + { + "title": "Courses", + "icon": "school", + "link": reverse_lazy( + "admin:courses_course_changelist" + ), + }, + { + "title": "Homeworks", + "icon": "assignment", + "link": reverse_lazy( + "admin:courses_homework_changelist" + ), + }, + { + "title": "Projects", + "icon": "folder_open", + "link": reverse_lazy( + "admin:courses_project_changelist" + ), + }, + { + "title": "Review criterias", + "icon": "checklist", + "link": reverse_lazy( + "admin:courses_reviewcriteria_changelist" + ), + }, + ], + }, + ], + }, +} diff --git a/courses/admin.py b/courses/admin.py deleted file mode 100644 index df64c93..0000000 --- a/courses/admin.py +++ /dev/null @@ -1 +0,0 @@ -from .admin import * \ No newline at end of file diff --git a/courses/admin/course.py b/courses/admin/course.py index 64385d5..86f12ac 100644 --- a/courses/admin/course.py +++ b/courses/admin/course.py @@ -1,5 +1,7 @@ +from typing import Any from django import forms from django.contrib import admin +from django.http import HttpRequest from django.utils import timezone from unfold.admin import ModelAdmin, TabularInline from unfold.widgets import ( @@ -9,6 +11,7 @@ from django.contrib import messages +from courses.mixin import InstructorAccessMixin from courses.models import Course, ReviewCriteria from courses.scoring import update_leaderboard @@ -98,7 +101,22 @@ def duplicate_course(modeladmin, request, queryset): @admin.register(Course) -class CourseAdmin(ModelAdmin): +class CourseAdmin(InstructorAccessMixin, ModelAdmin): actions = [update_leaderboard_admin, duplicate_course] inlines = [CriteriaInline] - list_display = ["title", "visible", "finished"] + list_display = ["title", "slug", "finished", "visible"] + + instructor_field = "instructor" + + def get_form( + self, + request: HttpRequest, + obj: Any | None = ..., + change: bool = ..., + **kwargs: Any, + ) -> forms.ModelForm: + form = super().get_form(request, obj, change, **kwargs) + if not request.user.is_superuser: + form.base_fields["instructor"].initial = request.user + form.base_fields["instructor"].widget = forms.HiddenInput() + return form diff --git a/courses/admin/homework.py b/courses/admin/homework.py index 4ba6114..0c1da8f 100644 --- a/courses/admin/homework.py +++ b/courses/admin/homework.py @@ -7,6 +7,7 @@ ) from django.contrib import messages +from courses.mixin import InstructorAccessMixin from courses.models import Homework, Question, HomeworkState from courses.scoring import ( @@ -95,7 +96,7 @@ def calculate_statistics_selected_homeworks( @admin.register(Homework) -class HomeworkAdmin(ModelAdmin): +class HomeworkAdmin(InstructorAccessMixin, ModelAdmin): inlines = [QuestionInline] actions = [ score_selected_homeworks, @@ -104,3 +105,5 @@ class HomeworkAdmin(ModelAdmin): ] list_display = ["title", "course", "due_date", "state"] list_filter = ["course__slug"] + + instructor_field = "course__instructor" diff --git a/courses/admin/projects.py b/courses/admin/projects.py index 7ec24d9..695e36c 100644 --- a/courses/admin/projects.py +++ b/courses/admin/projects.py @@ -3,6 +3,7 @@ from django.contrib import messages +from courses.mixin import InstructorAccessMixin from courses.models import Project, ReviewCriteria, ProjectState from courses.projects import ( @@ -77,7 +78,7 @@ def calculate_statistics_selected_projects( @admin.register(Project) -class ProjectAdmin(ModelAdmin): +class ProjectAdmin(InstructorAccessMixin, ModelAdmin): actions = [ assign_peer_reviews_for_project_admin, score_projects_admin, @@ -87,7 +88,12 @@ class ProjectAdmin(ModelAdmin): list_display = ["title", "course", "state"] list_filter = ["course__slug"] + instructor_field = "course__instructor" + @admin.register(ReviewCriteria) -class ReviewCriteriaAdmin(ModelAdmin): - pass +class ReviewCriteriaAdmin(InstructorAccessMixin, ModelAdmin): + list_display = ["course", "description", "review_criteria_type"] + list_filter = ["course"] + + instructor_field = "course__instructor" diff --git a/courses/migrations/0019_course_instructor.py b/courses/migrations/0019_course_instructor.py new file mode 100644 index 0000000..265cf1a --- /dev/null +++ b/courses/migrations/0019_course_instructor.py @@ -0,0 +1,27 @@ +# Generated by Django 4.2.14 on 2024-12-03 19:49 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ("courses", "0018_course_finished"), + ] + + operations = [ + migrations.AddField( + model_name="course", + name="instructor", + field=models.ForeignKey( + default=1, + on_delete=django.db.models.deletion.CASCADE, + related_name="courses_teaching", + to=settings.AUTH_USER_MODEL, + ), + preserve_default=False, + ), + ] diff --git a/courses/migrations/0023_merge_0019_course_instructor_0022_projectstatistics.py b/courses/migrations/0023_merge_0019_course_instructor_0022_projectstatistics.py new file mode 100644 index 0000000..46dc2be --- /dev/null +++ b/courses/migrations/0023_merge_0019_course_instructor_0022_projectstatistics.py @@ -0,0 +1,12 @@ +# Generated by Django 5.2.4 on 2025-10-04 18:21 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ('courses', '0019_course_instructor'), + ('courses', '0022_projectstatistics'), + ] + + operations = [] diff --git a/courses/mixin.py b/courses/mixin.py new file mode 100644 index 0000000..d47e6ed --- /dev/null +++ b/courses/mixin.py @@ -0,0 +1,46 @@ +class InstructorAccessMixin: + instructor_field = "instructor" + + def get_queryset(self, request): + """Filter queryset based on instructor field for non-superusers.""" + qs = super().get_queryset(request) + if not request.user.is_superuser: + return qs.filter(**{self.instructor_field: request.user}) + return qs + + def formfield_for_foreignkey( + self, db_field, request, obj=None, **kwargs + ): + """ + Filter foreign key querysets based on instructor field. + Supports both direct fields (e.g., 'instructor') and + related fields (e.g., 'course__instructor'). + """ + # Parse the instructor_field to handle relationships + parts = self.instructor_field.split("__") + formfield = super().formfield_for_foreignkey( + db_field, request, **kwargs + ) + # For direct instructor field on current model (no relationship traversal needed) + if len(parts) == 1: + return formfield + + # For related fields (e.g., course__instructor) + # parts[0] is the foreign key field name, parts[1:] is the lookup path + fk_field_name = parts[0] + lookup_path = "__".join(parts[1:]) + + queryset = formfield.queryset + + # Only apply filtering if this is the related foreign key field + if ( + db_field.name == fk_field_name + and not request.user.is_superuser + ): + kwargs["queryset"] = queryset.filter( + **{lookup_path: request.user} + ) + + return super().formfield_for_foreignkey( + db_field, request, **kwargs + ) diff --git a/courses/models/course.py b/courses/models/course.py index f0be4c5..d82e8d1 100644 --- a/courses/models/course.py +++ b/courses/models/course.py @@ -9,6 +9,7 @@ class Course(models.Model): + instructor = models.ForeignKey(User, on_delete=models.CASCADE, related_name="courses_teaching") slug = models.SlugField(unique=True, blank=False) title = models.CharField(max_length=200) diff --git a/courses/tests/test_instructor_mixin.py b/courses/tests/test_instructor_mixin.py new file mode 100644 index 0000000..6b6eac6 --- /dev/null +++ b/courses/tests/test_instructor_mixin.py @@ -0,0 +1,400 @@ +from django.test import TestCase, RequestFactory +from django.contrib.auth import get_user_model +from django.db import models +from unittest.mock import Mock, patch +from courses.mixin import InstructorAccessMixin + + +User = get_user_model() + + +# Mock models for testing +class MockInstructor(models.Model): + name = models.CharField(max_length=100) + + class Meta: + app_label = "test_app" + + +class MockCourse(models.Model): + instructor = models.ForeignKey( + MockInstructor, on_delete=models.CASCADE + ) + title = models.CharField(max_length=100) + + class Meta: + app_label = "test_app" + + +class MockLesson(models.Model): + course = models.ForeignKey(MockCourse, on_delete=models.CASCADE) + title = models.CharField(max_length=100) + + class Meta: + app_label = "test_app" + + +class TestInstructorAccessMixin(TestCase): + """Test cases for InstructorAccessMixin""" + + def setUp(self): + """Set up test fixtures""" + self.factory = RequestFactory() + self.superuser = Mock(spec=User) + self.superuser.is_superuser = True + + self.regular_user = Mock(spec=User) + self.regular_user.is_superuser = False + self.regular_user.id = 1 + + def test_get_queryset_superuser_returns_unfiltered(self): + """Superusers should see all objects""" + # Arrange + mock_qs = Mock() + + class ParentClass: + def get_queryset(self, request): + return mock_qs + + class TestMixin(InstructorAccessMixin, ParentClass): + pass + + mixin_instance = TestMixin() + mixin_instance.instructor_field = "instructor" + + request = self.factory.get("/") + request.user = self.superuser + + # Act + result = mixin_instance.get_queryset(request) + + # Assert + self.assertEqual(result, mock_qs) + mock_qs.filter.assert_not_called() + + def test_get_queryset_regular_user_filters_by_instructor(self): + """Regular users should only see their own objects""" + # Arrange + mock_qs = Mock() + mock_filtered_qs = Mock() + mock_qs.filter.return_value = mock_filtered_qs + + class ParentClass: + def get_queryset(self, request): + return mock_qs + + class TestMixin(InstructorAccessMixin, ParentClass): + pass + + mixin_instance = TestMixin() + mixin_instance.instructor_field = "instructor" + + request = self.factory.get("/") + request.user = self.regular_user + + # Act + result = mixin_instance.get_queryset(request) + + # Assert + mock_qs.filter.assert_called_once_with( + instructor=self.regular_user + ) + self.assertEqual(result, mock_filtered_qs) + + def test_get_queryset_with_related_field(self): + """Test filtering with related field like 'course__instructor'""" + # Arrange + mock_qs = Mock() + mock_filtered_qs = Mock() + mock_qs.filter.return_value = mock_filtered_qs + + class ParentClass: + def get_queryset(self, request): + return mock_qs + + class TestMixin(InstructorAccessMixin, ParentClass): + pass + + mixin_instance = TestMixin() + mixin_instance.instructor_field = "course__instructor" + + request = self.factory.get("/") + request.user = self.regular_user + + # Act + result = mixin_instance.get_queryset(request) + + # Assert + mock_qs.filter.assert_called_once_with( + course__instructor=self.regular_user + ) + self.assertEqual(result, mock_filtered_qs) + + def test_formfield_for_foreignkey_direct_field_no_filtering(self): + """Direct instructor field should not be filtered in formfield""" + # Arrange + mixin = InstructorAccessMixin() + mixin.instructor_field = "instructor" + + mock_db_field = Mock() + mock_db_field.name = "instructor" + + mock_super_result = Mock() + + class TestMixin(InstructorAccessMixin): + def __init__(self): + super().__init__() + self.instructor_field = "instructor" + + def formfield_for_foreignkey( + self, db_field, request, **kwargs + ): + if db_field.name == "instructor": + return mock_super_result + return super().formfield_for_foreignkey( + db_field, request, **kwargs + ) + + test_mixin = TestMixin() + request = self.factory.get("/") + request.user = self.regular_user + + # Act + result = test_mixin.formfield_for_foreignkey( + mock_db_field, request + ) + + # Assert + self.assertEqual(result, mock_super_result) + + def test_formfield_for_foreignkey_related_field_filters_for_regular_user( + self, + ): + """Related field should be filtered for non-superusers""" + # Arrange + mock_db_field = Mock() + mock_db_field.name = "course" + + mock_related_model = Mock() + mock_queryset = Mock() + mock_filtered_qs = Mock() + mock_queryset.filter.return_value = mock_filtered_qs + mock_related_model.objects = mock_queryset + + mock_remote_field = Mock() + mock_remote_field.model = mock_related_model + mock_db_field.remote_field = mock_remote_field + + mock_super_result = Mock() + + class TestMixin(InstructorAccessMixin): + def __init__(self): + super().__init__() + self.instructor_field = "course__instructor" + + def formfield_for_foreignkey( + self, db_field, request, obj=None, **kwargs + ): + # Call parent's implementation + parts = self.instructor_field.split("__") + if len(parts) > 1: + fk_field_name = parts[0] + lookup_path = "__".join(parts[1:]) + if ( + db_field.name == fk_field_name + and not request.user.is_superuser + ): + related_model = db_field.remote_field.model + kwargs["queryset"] = ( + related_model.objects.filter( + **{lookup_path: request.user} + ) + ) + return mock_super_result + + test_mixin = TestMixin() + request = self.factory.get("/") + request.user = self.regular_user + + # Act + result = test_mixin.formfield_for_foreignkey( + mock_db_field, request + ) + + # Assert + mock_queryset.filter.assert_called_once_with( + instructor=self.regular_user + ) + self.assertEqual(result, mock_super_result) + + def test_formfield_for_foreignkey_related_field_no_filter_for_superuser( + self, + ): + """Superuser should not have filtered queryset""" + # Arrange + mock_db_field = Mock() + mock_db_field.name = "course" + + mock_related_model = Mock() + mock_queryset = Mock() + mock_related_model.objects = mock_queryset + + mock_remote_field = Mock() + mock_remote_field.model = mock_related_model + mock_db_field.remote_field = mock_remote_field + + mock_super_result = Mock() + + class TestMixin(InstructorAccessMixin): + def __init__(self): + super().__init__() + self.instructor_field = "course__instructor" + + def formfield_for_foreignkey( + self, db_field, request, obj=None, **kwargs + ): + parts = self.instructor_field.split("__") + if len(parts) > 1: + fk_field_name = parts[0] + if ( + db_field.name == fk_field_name + and not request.user.is_superuser + ): + # This should not execute for superuser + kwargs["queryset"] = Mock() + return mock_super_result + + test_mixin = TestMixin() + request = self.factory.get("/") + request.user = self.superuser + + # Act + result = test_mixin.formfield_for_foreignkey( + mock_db_field, request + ) + + # Assert + mock_queryset.filter.assert_not_called() + self.assertEqual(result, mock_super_result) + + def test_formfield_for_foreignkey_different_field_not_filtered( + self, + ): + """Fields other than the instructor field should not be filtered""" + # Arrange + mock_db_field = Mock() + mock_db_field.name = ( + "other_field" # Different from instructor field + ) + + mock_super_result = Mock() + + class TestMixin(InstructorAccessMixin): + def __init__(self): + super().__init__() + self.instructor_field = "course__instructor" + + def formfield_for_foreignkey( + self, db_field, request, obj=None, **kwargs + ): + parts = self.instructor_field.split("__") + if len(parts) > 1: + fk_field_name = parts[0] + if db_field.name == fk_field_name: + # This should not execute + kwargs["queryset"] = Mock() + return mock_super_result + + test_mixin = TestMixin() + request = self.factory.get("/") + request.user = self.regular_user + + # Act + result = test_mixin.formfield_for_foreignkey( + mock_db_field, request + ) + + # Assert + self.assertEqual(result, mock_super_result) + self.assertNotIn("queryset", {}) # queryset should not be set + + def test_instructor_field_with_deep_relationship(self): + """Test with deeply nested relationship like 'course__department__instructor'""" + # Arrange + mock_qs = Mock() + mock_filtered_qs = Mock() + mock_qs.filter.return_value = mock_filtered_qs + + class ParentClass: + def get_queryset(self, request): + return mock_qs + + class TestMixin(InstructorAccessMixin, ParentClass): + pass + + mixin_instance = TestMixin() + mixin_instance.instructor_field = ( + "course__department__instructor" + ) + + request = self.factory.get("/") + request.user = self.regular_user + + # Act + result = mixin_instance.get_queryset(request) + + # Assert + mock_qs.filter.assert_called_once_with( + course__department__instructor=self.regular_user + ) + self.assertEqual(result, mock_filtered_qs) + + +class TestInstructorAccessMixinIntegration(TestCase): + """Integration tests with actual Django admin""" + + def setUp(self): + self.factory = RequestFactory() + + def test_mixin_with_model_admin(self): + """Test that mixin works correctly when combined with ModelAdmin""" + + # Create a simple parent class that mimics ModelAdmin behavior + class MockModelAdmin: + def __init__(self): + self.model = Mock() + self.opts = Mock() + + def get_queryset(self, request): + return Mock() + + def formfield_for_foreignkey( + self, db_field, request, **kwargs + ): + return Mock() + + class TestAdmin(InstructorAccessMixin, MockModelAdmin): + instructor_field = "instructor" + + admin = TestAdmin() + + # Test that the mixin methods are available + self.assertTrue(hasattr(admin, "get_queryset")) + self.assertTrue(hasattr(admin, "formfield_for_foreignkey")) + self.assertEqual(admin.instructor_field, "instructor") + + # Test that get_queryset works + request = self.factory.get("/") + request.user = Mock() + request.user.is_superuser = False + + mock_qs = Mock() + mock_filtered_qs = Mock() + mock_qs.filter.return_value = mock_filtered_qs + + # Override the parent's get_queryset to return our mock + with patch.object( + MockModelAdmin, "get_queryset", return_value=mock_qs + ): + parent_qs = MockModelAdmin().get_queryset(request) + self.assertEqual(parent_qs, mock_qs)